I’ve been reading these for a while now and I almost feel like I’m interrupting but just wanted to let you know that I admire the amount of work that you’ve put into this series!
2 Likes
Converting the tests to get them working with webpack and karma is what we’re doing next.
Removing unwanted scripts
After having got webpack+babel+karma+jasmine working, I now feel good enough about things that we can remove the testing scripts, and use a proper script for using karma tests instead.
package.json
"scripts": {
...
// "test:mocha": "mocha --require @babel/register test/mocha.test",
// "test:karma": "karma start",
// "test:webpack": "mocha-webpack test/test.js",
// "test:sandbox": "mocha --require @babel/register test/sandbox.test"
"test": "karma start"
},
We can also now remove the mocha.test.js, test.js, and jasmine.test.js files.
The improved testing
Instead of using npm run test:karma, we now have a much shorter way to start the tests:
> npm test
and the tests keep on running whenever changes are saved to a file.
Migrate the tests over to webpack
The sandbox.test.js test now works, so we can start converting the other test files.
As Karma is setup to test files ending with test.js, I can rename the *.spec.js files to *.test.js as I migrate the code. That will help to keep track of what’s been converted, and what yet remains to be done.
Convert sandbox
Most of the tests rely on the sandbox code which provides a nice and consistent set of HTML code for testing against, so that’s the easiest first test to convert.
We can rename sandbox.spec.js to sandbox.test.js, which helps us to keep the other tests out of the way until we’re ready to deal with them.
The define statements are replaced with import statements instead. Because import statements are used, we don’t need the use strict declaration anymore either.
sandbox.test.js
// define(["../tests/sandbox"], function (sandbox) {
import sandbox from "../tests/sandbox";
// "use strict";
...
// });
And with the sandbox code, we just need to export is so that it can be imported by the test.
sandbox.js
// define([], function () {
// "use strict";
...
// return {
export default {
...
};
// });
And the test now works properly.
Convert create-sandbox
Rename createsandbox.spec.js to createsandbox.test.js, and add the import statement:
create-sandbox.test.js
// describe("Create-sandbox", function (name) {
import sandbox from "./create-sandbox";
// "use strict";
...
// });
create-sandbox.js
// define(["../tests/sandbox"], function (sandbox) {
import sandbox from "./sandbox";
// "use strict";
// });
export default sandbox;
We then get a testing error because we are no longer using the Jasmine test runner and are now using Karma instead.
TypeError: Cannot read property 'nextSibling' of null
The code responsible for that is some debugging code that we no longer have a use for, as we’re not using the test runner anymore, so it can be removed from the test.
create-sandbox.test.js
// afterAll(function () {
// var reporter = document.querySelector(".jasmine_html-reporter");
// if (reporter.nextSibling) {
// console.log("Test has failed to clean up after itself.", reporter.nextSibling);
// }
// });
And the above tests now run with no problem. We can start converting other tests over too.
convert scroll
The scroll test is the next simplest one to convert.
scroll.test.js
// define(["../tests/sandbox", "scroll"], function (sandbox, scroll) {
import sandbox from "./sandbox";
import scroll from "../src/scroll";
// "use strict";
...
// });
scroll.js
// return {
export default {
This test didn’t take much at all to get working.
Next steps
The remaining code tends to rely on jQuery plugins, so dealing with those will be the focus of the next post.
2 Likes
I’ve been trying several ideas to deal with jQuery plugins, but nothing seems to work when it comes to using the jquery.isscrollable plugin code with webpack+karma.
For example, these are some of the resources that have been tried.
Maybe it’s because the isscrollable extension doesn’t load as a node module, or maybe it’s because the file is misnamed being called “jquey.is-scrollable.js” instead.
Whatever the cause (I will explore this later for the sake of learning), it’s high time to rip it out of our code, which also happens to align nicely with earlier ideas to rid our code of jQuery.
It’s not that jQuery is bad, but it’s bloated for what we use of it. Eventually we’ll be able to remove jQuery completely, helping to trim some weight from our code too.
The choices to be made
Currently there are several balls that need to be juggled at the same time.
- most of the tests haven’t yet been converted over
- some parts of the code haven’t been fully integrated yet
- code is being changed with no good way to tell if things still work
I don’t like making big changes without tests to tell us if the code is still working.
I have a few different options for progress from here.
- bash my head further against the seemingly brick-wall of integrating jquery plugins into webpack & karma
- just remove the plugins no tests as a safety net
- remove jquery plugins from previous working code, then migrate over the updated code
Somehow I find that the latter option is the more pleasing one to consider.
Currently the jquery plugins are a blocker to further progress. Either I get them working, ignore them potentially at my peril, or remove them.
If I do suceed at getting them working, I then plan to remove them. So removing them is the best option here.
The trouble is though, that I can’t reliably go ahead with making such changes without a good set of tests to ensure that nothing breaks.
What I can do instead is to go back to previous code which has working tests, and use that to remove the jQuery plugins, testing all the while to ensure that nothing breaks.
Stop using the isscrollable plugin
I’ve created a separate sr-beta\ folder that contains an earlier version of the code with working tests. I can monitor those tests while I remove the jQuery plugins.
With that earlier set of code, we can update the code so that only the element is passed instead:
scroll.js
// var $container = getScrollContainer($el);
var $container = getScrollContainer($el.get(0));
...
// function getScrollContainer($el) {
function getScrollContainer(el) {
// return $el.scrollableparent();
return $(el).scrollableparent();
}
We can now use a local function that finds out if the element is scrollable or not.
scroll.js
function isScrollable(el) {
var style = window.getComputedStyle(el);
return (
style.overflow === "scroll" ||
style.overflow === "auto" ||
style.overflowX === "scroll" ||
style.overflowX === "auto" ||
style.overflowY === "scroll" ||
style.overflowY === "auto"
);
}
function getScrollContainer(el) {
var parent = el.parentNode;
while (!isScrollable(parent) && parent.nodeName !== "HTML") {
parent = parent.parentNode;
}
return $(parent);
}
Prevent other code from passing jQuery objects to getScrollContainer
Everything else that calls getScrollContainer needs to only pass in the non-jquery element too.
To ensure that I catch everything, I’m going to break out of the code when a jQuery object is noticed being passed to the function.
scroll.js
function getScrollContainer(el) {
if (el.constructor.name === "jQuery") {
debugger;
}
That way, I can just rerun the tests and when the code breaks into the debugger, I can tell from the callstack exactly where the next update needs to occur.
scrollmanager.js
function hasScrollContainerAsParent($el) {
var el = $($el).get(0);
// var scrollContainer = scroll.getScrollContainer($el);
var scrollContainer = scroll.getScrollContainer(el);
...
function getPageUpItem($el) {
var el = $($el).get(0);
// var $container = scroll.getScrollContainer($el);
var $container = scroll.getScrollContainer(el);
...
function getPageDownItem($el) {
var el = $($el).get(0);
// var $container = scroll.getScrollContainer($el);
var $container = scroll.getScrollContainer(el);
scroll.spec.js
// var scrollContainer = scroll.getScrollContainer($(item));
var scrollContainer = scroll.getScrollContainer($(item).get(0));
And lastly, there is the getContainerChild function:
scrollmanager.js
// isScrollParent = scroll.getScrollContainer($($el).get(0)).is($el.parent());
isScrollParent = scroll.getScrollContainer($el).is($el.parent());
The getScrollContainer function can now return just a normal element, which we’ll convert back that back to a jQuery one when we call the function:
scroll.js
function getScrollContainer(el) {
...
// return $(parent);
return parent;
}
// var $container = getScrollContainer($el.get(0));
var $container = $(getScrollContainer($el.get(0)));
scrollmanager.js
// var scrollContainer = scroll.getScrollContainer(el);
var scrollContainer = $(scroll.getScrollContainer(el));
...
// if (scroll.getScrollContainer($($el).get(0)).is($el.parent())) {
if ($(scroll.getScrollContainer($($el).get(0))).is($el.parent())) {
...
// var $container = scroll.getScrollContainer(el);
var $container = $(scroll.getScrollContainer(el));
Remove the last traces of the jquery.isscrollable plugin
The above changes have resulted in jQuery being pushed up to the getContainerChild function, and we can now remove jquery.isscrollable from the code:
// define(["jquery", "jquery.isscrollable"], function ($) {
define(["jquery"], function ($) {
It’s not needed now in the main.js files too.
tests/main.js and src/main.js
paths: {
...
// "jquery.isscrollable": "../lib/jquery.isscrollable",
...
},
shim: {
...
"jquery.isscrollable": ["jquery"],
...
}
Next steps
I noticed earlier that there is a good opportunity with the getContainerChild function to explore what needs to be done when removing jQuery entirely. I’ll explore that next, before moving on to removing other jQuery plugins.
2 Likes
I intend to eventually remove jQuery from all of our code, for it’s a huge library that our code doesn’t benefit from.
Trust me on this, it’ll be worth it.
Remove jQuery from getContainerChild
Here’s the initial function as it currently stands:
scrollmanager.js
function getContainerChild($el) {
var isScrollParent = false;
do {
$el = $el.parent();
isScrollParent = $(scroll.getScrollContainer($($el).get(0))).is($el.parent());
} while (!isScrollParent && !$el.is("html"));
return $el;
}
This is a more complex example because of the loop. Most of the time it’s as easy as just stopping jQuery coming in and going out, then converting the rest of the function.
Here’s how we remove the need for jQuery from this function. First I make sure that things keep working when only HTML elements are coming into the function:
scrollmanager.js
function getContainerChild(el) {
var $el = $(el);
and I must ensure that other functions that use getContainerChild only pass normal elements without jQuery.
scrollmanager.js
function createInsideAboveFilter($el, scrollHeight) {
// $el = getContainerChild($el);
$el = getContainerChild($($el).get(0));
...
function createInsideBelowFilter($el, scrollHeight) {
// $el = getContainerChild($el);
$el = getContainerChild($($el).get(0));
If I just used $el.get(0) then that would fail if $el was an element, so by turning it into a jQuery object then getting an element from it, I can then later on stop other functions from giving jQuery objects to this function and then replace that whole conversion with just el.
My focus must not be on these filter functions though. For now it’s on the getContainerChild function.
I can now simplify things in the getContainerChild function by replacing the do…while loop with a simple while loop:
scrollmanager.js
// do {
while (!isScrollParent && $el.is("html")) {
$el = $el.parent();
isScrollParent = $(scroll.getScrollContainer($($el).get(0))).is($el.parent());
// } while (!isScrollParent && !$el.is("html"));
}
The tests still pass which helps to reassure me that the updated loop is still correct.
I can then simplify things even further by using an isScrollParent function instead:
scrollmanager.js
function isScrollParent(el) {
var container = scroll.getScrollContainer(el);
return container === el.parentNode;
}
function getContainerChild(el) {
// var isScrollParent = false;
while (!isScrollParent($($el).get(0)) && !$el.is("html")) {
$el = $el.parent();
// isScrollParent = $(scroll.getScrollContainer($($el).get(0))).is($el.parent());
}
I can now convert $el statements to just el, and use normal element handling techniques with the remainder of the code:
scrollmanager.js
// var $el = $(el);
// while (!scrollParent($($el).get(0)) && !$el.is("html")) {
// $el = $el.parent();
}
// return $el;
// return $(el);
while (!scrollParent(el) && el.nodeName !== "HTML") {
el = el.parentNode;
}
return el;
After a few more refinements we end up with the following final code:
function getContainerChild(item) {
var container = item;
while (!isScrollParent(container) && container.nodeName !== "HTML") {
container = container.parentNode;
}
return container;
}
Let’s compare that improved code with the original function that used the jQuery library:
scrollmanager.js
function getContainerChild($el) {
var isScrollParent = false;
do {
$el = $el.parent();
isScrollParent = $(scroll.getScrollContainer($($el).get(0))).is($el.parent());
} while (!isScrollParent && !$el.is("html"));
return $el;
}
That jQuery code is now seen to be not as good as the updated code just above it. Admittedly, even though it might be possible with more work for the jQuery code to do just as good a job, it’s not worth the bloat.
Next steps
By a gradual process of removing jQuery from functions and pushing it out to other functions that call them, we’ll eventually be able to remove jQuery completely.
More of that can wait though until after the other jQuery plugins have been removed, which I’ll do in the next post.
2 Likes
Remove jquery.focusable
The key place where the jquery.focusable plugin is used is in the setFocused function.
scrollmanager.js
function setFocused($el) {
if ($el.focusable().length > 0) {
$el = $el.focusable();
}
$el.focus();
}
The focusable plugin doesn’t tell you if an element is focusable.
Instead, it searches for any children that are focusable.
We can achieve the same thing with the following code:
scrollmanager.js
function getFocusable(parent) {
return parent.querySelectorAll(
"button, [href], input, select, textarea, " +
"[tabindex]:not([tabindex='-1'])"
);
}
So first, we prevent other code from sending jQuery objects to the setFocused function:
scrollmanager.js
// function setFocused($el) {
function setFocused(el) {
var $el = $(el);
...
}
function moveTo(el) {
...
// setFocused($el);
setFocused($($el).get(0));
}
We can now replace the jQuery-specific code in the setFocused function:
scrollmanager.js
// var $el = $(el);
// if ($el.focusable().length > 0) {
// $el = $el.focusable();
// }
// $el.focus();
var focusable = getFocusable(el);
if (focusable) {
focusable[0].focus();
}
And we’re done! We can now remove the jquery.focusable plugin
scrollmanager.js
// define(["jquery", "scroll", "jquery.focusable"], function ($, scroll) {
define(["jquery", "scroll"], function ($, scroll) {
src/main.js and tests/main.js
paths: {
...
// "jquery.focusable": "../lib/jquery.focusable",
...
},
shim: {
// "jquery.focusable": ["jquery"]
}
Migrate code
We should now be ready to migrate the updated code. The files that we’ve changed are:
- scroll.js
- scroll.spec.js
- scrollmanager.js
- tests/main.js
- src/main.js
The main files aren’t of any relevance to webpack, so back with the webpack code I can rename the scroll and scrollmanager files to scroll-old.js, scroll.test-old.js, and scrollmanager-old.js, and copy over the updated scroll and scrollmanager code.
And Karma successfully runs the scroll tests on the scroll code, I can now remove the old version of those updated files, and carry on with converting the test files.
Next steps
Before converting code so that jQuery isn’t used, I want all of the tests to be in place so that we can easily watch for problems. So next time, I’ll get the other tests working with webpack+karma too.
2 Likes
Now that the complicating jQuery plugins aren’t being used, I want all of the tests to be properly working so that we can easily watch for problems as we work on the code.
Converting tests
The order of converting the tests seems to be best achieved in the following order:
- scrollmanager.test.js
- checkboxlist.test.js
- resultscontroller.test.js
- filtercheckboxesbytext.test.js
- searchresults.test.js
- createcategories.test.js
- createresults.test.js
Converting the tests
The conversion process is a simple task, where node_modules have no path, local files have ./ for their path, and other files are accessed by reference from the test folder, such as with ../src/
scrollmanager.test.js
// define(["../tests/sandbox", "scrollmanager"], function (sandbox, scrollManager) {
import sandbox from "./create-sandbox";
import scrollManager from "../src/scrollmanager";
// "use strict";
...
// });
I can modify the karma config to individually check that the tests are working:
karma.config.js
files: [
// "test/**/*test.js",
"test/scrollmanager.test.js",
...
],
which confirms that the scrollmanager test is all working.
Repeat for all tests
Repeating the above on all of the other tests has them all working, and I can now put the karma config back to what it normally should be:
karma.config.js
files: [
// "test/createresults.test.js",
"test/**/*test.js",
...
],
and all of the tests now work in the webpack+karma environment!
We’re making good progress on the TODO list:
- Get code working
- Remove requirejs
- Convert to import/export
- Remove aliases
- Get tests working with webpack
- Remove global jQuery
Next steps
Next time, we get rid of jQuery.
2 Likes
jQuery has been useful at giving us a few easy ways to do things, but it comes at the cost of its enormous size.
Removing jQuery helps to give us a dramatically smaller size for our code.
Removing jQuery from scrollmanager
When it comes to removing the need for jQuery code, it helps if other code doesn’t expect a jQuery object from scrollmanager. When checking for this, I find that that none of the code that calls scrollManager saves any information from it, so we don’t have to worry about that particular issue.
The other issue is passing information to scrollmanager. I want no jQuery objects being given to the scrollmanager code either. We can instead convert them to normal elements, before we convert things further.
checkboxlist.js
function checkboxClickHandler(evt, afterClick) {
...
// scrollManager.moveTo($(el));
scrollManager.moveTo(el);
...
}
scrollmanager.js
// function moveTo($item) {
function moveTo(item) {
$item = $(item);
...
}
Fortunately we have the tests that rapidly tell us when things stop working properly, which allows me to either undo back to working code, or keep me informed about the next simple thing that needs to be fixed.
Removing jQuery ins and outs with hasScrollContainerAsParent
Starting from the top of the code, we have
scrollmanager.js
function hasScrollContainerAsParent($el) {
var $scrollContainer = $(scroll.getScrollContainer($el));
var $parent = $el.parent();
return $scrollContainer.is($parent);
}
First we investigate function’s return value and input parameters. It has a boolean return so we don’t need to worry about how other code handles the return value.
The function parameter though needs to be an element instead of a jQuery object. Fortunately, I made sure to prefix all jQuery variables with a dollar symbol, helping us to easily recognise them.
The code that calls hasScrollContainerAsParent needs to be updated so that it just gives an element instead.
scrollmanager.js
function hasScrollContainerAsParent(el) {
var $el = $(el);
...
}
// while (!hasScrollContainerAsParent($el) && !$el.is("html")) {
while (!hasScrollContainerAsParent($el.get(0)) && !$el.is("html")) {
Removing jQuery from calls to the scroll code
We call scroll.getScrollContainer with a jQuery object. We should do that with a normal element, adjusting both scrollmanager and scroll with a minimum of changes so that they both continue to work:
scrollmanager.js
function hasScrollContainerAsParent($el) {
// var scrollContainer = $(scroll.getScrollContainer($el));
var $scrollContainer = $(scroll.getScrollContainer($el));
...
}
scroll.js
function getScrollContainer(item) {
$item = $(item);
...
}
We achieve a clean separation by removing jQuery from the ins and outs of the function, , and can now focus on converting the scrollmanager code without having to worry about how it impacts other parts of the code.
Completely removing jQuery from hasScrollContainerAsParent
We now have no jQuery objects coming in or going out of the function, so we can now happily convert it to work with normal elements.
scrollmanager.js
function hasScrollContainerAsParent(el) {
// var $el = $(el);
// var $scrollContainer = $(scroll.getScrollContainer($el));
var scrollContainer = scroll.getScrollContainer(el);
// var $parent = $el.parent();
var parent = el.parentNode;
// return $scrollContainer.is($parent);
return scrollContainer === parent;
}
We can even move that parentNode line down to the return statement:
scrollmanager.js
function hasScrollContainerAsParent(el) {
var scrollContainer = scroll.getScrollContainer(el);
// var parent = el.parentNode;
return scrollContainer === el.parentNode;
}
And that function is now completely free of jQuery.
Repeat ad-nauseum in scrollmanager code
It’s just a matter of doing that to the rest of scrollmanager functions now, for which I’ll spare you the details.
About the only addition that was needed was a way to get sibling elements, for which I added the following code:
scrollmanager.js
function getSiblings(el, type) {
var siblings = [];
var siblingType = (type === "prev")
? "previousElementSibling"
: "nextElementSibling";
el = el[siblingType];
while (el) {
siblings.push(el);
el = el[siblingType];
}
return siblings;
}
The scrollmanager code without jQuery looks remarkably like what we were using with jQuery. Here’s the full scrollmanager code.
scrollmanager.js
/*jslint browser */
import scroll from "./scroll";
function getSiblings(el, type) {
var siblings = [];
var siblingType = (type === "prev")
? "previousElementSibling"
: "nextElementSibling";
el = el[siblingType];
while (el) {
siblings.push(el);
el = el[siblingType];
}
return siblings;
}
function hasScrollContainerAsParent(item) {
var scrollContainer = scroll.getScrollContainer(item);
return scrollContainer === item.parentNode;
}
function upToScrollChild(item) {
var container = item;
while (!hasScrollContainerAsParent(container) && container.nodeName !== "HTML") {
container = container.parentNode;
}
return container;
}
function scrollTo(item) {
scroll.intoView(item);
}
function getFocusableFrom(container) {
return container.querySelectorAll(
"button, [href], input, select, textarea, " +
"[tabindex]:not([tabindex='-1'])"
);
}
function setFocused(item) {
var focusable = getFocusableFrom(item);
if (focusable) {
focusable[0].focus();
}
}
function moveTo(item) {
item = upToScrollChild(item);
scrollTo(item);
setFocused(item);
}
function moveToGivenDirection(item, getElFunc) {
var scrollChild = item;
if (!hasScrollContainerAsParent(item)) {
scrollChild = upToScrollChild(item);
}
scrollChild = getElFunc(scrollChild);
if (!scrollChild) {
scrollChild = item;
}
moveTo(scrollChild);
}
function moveToPrevious(item) {
moveToGivenDirection(item, function getPrev(item) {
return visibleSiblings(item, "prev")[0];
});
}
function moveToNext(item) {
return moveToGivenDirection(item, function getNext(item) {
return visibleSiblings(item, "next")[0];
});
}
function isScrollParent(el) {
var scrollParent = scroll.getScrollContainer(el);
return el.parentNode === scrollParent;
}
function getContainerChild(item) {
var container = item;
while (!isScrollParent(container) && container.nodeName !== "HTML") {
container = container.parentNode;
}
return container;
}
function createInsideAboveFilter(item, scrollHeight) {
var container = getContainerChild(item);
return function scrollViewFilter(child) {
var dist = scroll.outerDistanceBetween(container, child);
return dist > 0 && dist < scrollHeight;
};
}
function createInsideBelowFilter(item, scrollHeight) {
var container = getContainerChild(item);
return function scrollViewFilter(child) {
var dist = scroll.outerDistanceBetween(child, container);
return dist > 0 && dist < scrollHeight;
};
}
function getPageUpItem(item) {
var container = scroll.getScrollContainer(item);
var items = Array.from(container.children);
var scrollHeight = scroll.innerHeight(container);
var scrollViewFilter = createInsideAboveFilter(item, scrollHeight);
var filteredItems = items.filter(scrollViewFilter);
return filteredItems[0];
}
function getPageDownItem(item) {
var container = scroll.getScrollContainer(item);
var items = Array.from(container.children);
var scrollHeight = scroll.innerHeight(container);
var scrollViewFilter = createInsideBelowFilter(item, scrollHeight);
var filteredItems = items.filter(scrollViewFilter);
return filteredItems.pop();
}
function pageUpFrom(item) {
item = getPageUpItem(item);
return moveTo(item);
}
function pageDownFrom(item) {
item = getPageDownItem(item);
return moveTo(item);
}
function moveToStart(item) {
var scrollChild;
if (!hasScrollContainerAsParent(item)) {
scrollChild = upToScrollChild(item);
}
scrollChild = visibleSiblings(scrollChild, "prev").pop();
if (!scrollChild) {
scrollChild = item;
}
return moveTo(scrollChild);
}
function moveToEnd(item) {
var scrollChild;
if (!hasScrollContainerAsParent(item)) {
scrollChild = upToScrollChild(item);
}
scrollChild = visibleSiblings(scrollChild, "next").pop();
if (!scrollChild) {
scrollChild = item;
}
return moveTo(scrollChild);
}
function keydownHandler(evt) {
var item = evt.target;
if (evt.key === "PageUp") {
pageUpFrom(item);
evt.preventDefault();
}
if (evt.key === "PageDown") {
pageDownFrom(item);
evt.preventDefault();
}
if (evt.key === "End") {
moveToEnd(item);
evt.preventDefault();
}
if (evt.key === "Home") {
moveToStart(item);
evt.preventDefault();
}
if (evt.key === "ArrowUp") {
moveToPrevious(item);
evt.preventDefault();
}
if (evt.key === "ArrowDown") {
moveToNext(item);
evt.preventDefault();
}
}
export default {
moveTo: moveTo,
keydownHandler: keydownHandler
};
Next steps
I know that I’ll need that visibleSiblings in other code, but will wait until it’s duplicated in at least one other place yet, before moving it out to a utils import.
We’ve made a good start at removing our reliance on jQuery, and can now move on with removing it from the other sets of code.
- Remove aliases
- Get tests working with webpack
- Remove jQuery from:
- scrollmanager.js
- scroll.js
- checkboxlist.js
- filtercheckboxesbytext.js
- resultscontroller.js
- searchresults.js
- filtercheckboxesbytext.test.js
- resultscontroller.test.js
- scrollmanager.js
- Remove global jQuery
I like think of this as being similar to removing twitch from the garden. After doing the above tasks jQuery will be completely removed, won’t need it there any more, and we can remove its gigantic weight from our project.
2 Likes
The above process to remove jQuery was followed with the scroll code, which just needed a simple innerHeight function:
scroll.js
function innerHeight(el) {
var style = window.getComputedStyle(el);
return parseInt(style.height);
}
and the rest of it was easy to convert, resulting in the following code:
scroll.js
/*jslint browser */
function innerHeight(item) {
var style = window.getComputedStyle(item);
return parseInt(style.height);
}
function scrollUpDifference(item, container) {
var offsetFromContainerTop = item.offsetTop - container.offsetTop;
var desiredOffset = innerHeight(container) - item.offsetHeight;
return offsetFromContainerTop - desiredOffset;
}
function scrollUpTo(item, container) {
var offsetDifference = scrollUpDifference(item, container);
container.scrollTop = container.scrollTop + offsetDifference;
}
function scrollDownDifference(item, container) {
var containerScroll = container.scrollTop + container.offsetTop;
return item.offsetTop - containerScroll;
}
function scrollDownTo(item, container) {
var offsetDifference = scrollDownDifference(item, container);
container.scrollTop = container.scrollTop + offsetDifference;
}
function isScrollable(item) {
var style = window.getComputedStyle(item);
return (
style.overflow === "scroll" ||
style.overflow === "auto" ||
style.overflowX === "scroll" ||
style.overflowX === "auto" ||
style.overflowY === "scroll" ||
style.overflowY === "auto"
);
}
function getScrollContainer(item) {
var parent = item.parentNode;
while (!isScrollable(parent) && parent.nodeName !== "HTML") {
parent = parent.parentNode;
}
return parent;
}
function outerHeight(item) {
return item.offsetHeight;
}
function scrollIntoView(item) {
var container = getScrollContainer(item);
if (scrollUpDifference(item, container) > 0) {
scrollUpTo(item, container);
}
if (scrollDownDifference(item, container) < 0) {
scrollDownTo(item, container);
}
}
function distanceBetween(item1, item2) {
return item1.offsetTop - item2.offsetTop;
}
function outerDistanceBetween(item1, item2) {
var dist = distanceBetween(item1, item2);
dist += (dist >= 0)
? outerHeight(item1)
: outerHeight(item2);
return dist;
}
export default {
intoView: scrollIntoView,
outerDistanceBetween: outerDistanceBetween,
innerHeight: innerHeight,
getScrollContainer: getScrollContainer
};
jQuery was also removed from Checkboxlist very easily, and found that I was duplicating some functions so they’ve been put in to a separate utils file:
utils.js
function isVisible(el) {
return el.offsetParent !== null;
}
function getVisible(els) {
return Array.from(els).filter(isVisible);
}
export default {
isVisible,
getVisible
};
checkboxlist.js
/*jslint browser */
import utils from "./utils";
import scrollManager from "./scrollmanager";
var container;
var clickHandler;
function getCheckboxByValue(value) {
return document.querySelector("input[value=" + value + "]");
}
function uncheckByValue(value) {
var checkbox = getCheckboxByValue(value);
if (checkbox.checked) {
checkbox.click();
}
}
// We can't just set the checkbox checked value to true.
// Other things also need to occur that are triggered by the click event.
// Because of that, we need to click on the checkbox instead.
function checkFirstCheckbox() {
var checkboxes = container.querySelectorAll("input[type=checkbox]");
var firstCheckbox = utils.getVisible(checkboxes)[0];
if (!firstCheckbox.checked) {
firstCheckbox.click();
}
firstCheckbox.focus();
}
function reset() {
var inputs = container.querySelectorAll("input[type=checkbox]");
var checkedInputs = Array.from(inputs).filter((input) => input.checked);
checkedInputs.forEach((input) => input.click());
}
function checkboxClickHandler(evt, afterClick) {
var el = evt.target;
scrollManager.moveTo(el);
afterClick(el);
}
function clickWrapper(callback) {
return function wrappedClickHandler(evt) {
checkboxClickHandler(evt, callback);
};
}
function init(opts) {
container = opts.container;
clickHandler = clickWrapper(opts.afterClick);
container.addEventListener("click", function (evt) {
var el = evt.target;
if (el.type === "checkbox") {
return clickHandler(evt);
}
});
container.addEventListener("keydown", scrollManager.keydownHandler);
}
export default {
uncheckByValue: uncheckByValue,
checkFirst: checkFirstCheckbox,
reset: reset,
init: init
};
The comment that I left in the above code reminds me that I also need to revisit the checkbox click handler code. It would be nice if it could store and run a series of events from an array instead.
Next steps
The process of removing jQuery is progressing smoothly, and at this pace shouldn’t take long to complete.
- Remove jQuery from:
- scrollmanager.js
- scroll.js
- checkboxlist.js
- filtercheckboxesbytext.js
- resultscontroller.js
- searchresults.js
- filtercheckboxesbytext.test.js
- resultscontroller.test.js
- scrollmanager.js
- Remove global jQuery
- Use click handler events array
2 Likes
Continuing on with removing the need for jQuery in our code, we’re removing it from the rest of the code.
Remove jQuery from filtercheckboxesbytext.test.js
This test was causing an error to occur because a searchField variable was undefined.
This one was easy to get working, just by checking that a searchfield exists before adding the event listener to it.
filtercheckboxesbytext.js
if (searchField) {
searchField.addEventListener("keyup", function searchHandler(evt) {
...
}
}
However, I’m not happy about needing to make that kind of check. It’s only there because the createresults tests use searchresults.init() when no search field exists.
It becomes clear that the createresults tests shouldn’t touch searchresults. Those tests are less to do with createresults, and more with what searchresults needs to do instead. Because of this those tests are moved out of createresults and over to the searchresults code instead.
searchresults.test.js
it("initializes createResults", function () {
window.spyOn(createResults, "init");
searchResults.init();
expect(createResults.init).toHaveBeenCalled();
});
Other parts of the test are easily dealt with too just by renaming searchresults to createresults instead.
createresults.test.js
// searchResults.init(catData);
createResults.init(catData);
results = document.querySelector("#results");
expect(results).not.toBeNull();
After making similar adustments to createcategories tests and createresults tests, the if statement is now not needed at all and can be removed.
filtercheckboxesbytext.js
// if (searchField) {
searchField.addEventListener("keyup", function searchHandler(evt) {
...
// }
The full filtercheckboxesbytext code is:
filtercheckboxesbytext.js
/*jslint browser */
import escapeRegExp from "escape-regexp";
export default function filterCheckboxesByText(container, searchField) {
var searchValue;
function containsText(haystack, needle) {
return needle.toLowerCase().indexOf(haystack.toLowerCase()) > -1;
}
function compareWithSearch(checkbox) {
var label = checkbox.nextElementSibling;
return containsText(searchValue, label.innerText);
}
function caseInsensitiveBold(text, search) {
var searchRx = new RegExp("(" + escapeRegExp(search) + ")", "gi");
return text.replace(searchRx, "<b>$1</b>");
}
function highlightMatches(search, container) {
container.querySelectorAll("label").forEach(function (label) {
label.innerHTML = caseInsensitiveBold(label.innerText, search);
});
}
function showItem(checkbox) {
var parent = checkbox.parentNode;
parent.style.display = "";
parent.classList.remove("hide");
}
searchField.addEventListener("keyup", function searchHandler(evt) {
searchValue = evt.target.value;
Array.from(container.querySelectorAll("p")).forEach(
(para) => para.classList.add("hide")
);
var checkboxes = container.querySelectorAll("input[type=checkbox]");
Array.from(checkboxes).filter(compareWithSearch).forEach(showItem);
highlightMatches(searchValue, container);
});
};
While testing the above filter code I noticed that there were some usability issues that need to be fixed, such as when the clear all button clears the text but fails clear the filter. and another issue where on clearing that the text entry focus should go back to the search bar. I’ll work on those at a later time after the current task is complete, and will add it to the todo list.
Remove jQuery from resultscontroller.js
This code is the first that gives me big conversion troubles, as the way that jQuery hides elements differs from the recommended technique that JavaScript should use when working with HTML.
jQuery sets the style display to block/none. Instead of messing with the details of styles, a more recommended technique is to use a class name instead, such as hide
To help deal with the conflicting situations, I ended up using if statements to separate the jQuery command, which helped me to understand more about what was happening.
resultscontroller.js
// $(result).toggle(state);
if (state === true) {
$(result).show();
}
if (state === false) {
$(result).hide()
}
if (state === undefined) {
$(result).toggle(state);
}
And found that the undefined state meant that I wasn’t able to easily achieve anything simple and comparable, until I looked at the CSS code. The CSS code was hiding the results section.
There’s a general rule where when you are using JavaScript to hide/show elements, it’s best to not have CSS confusing things by muddying the mix.
Success was achieved for one of the situations by removing that CSS code, and having the init section hide the results instead.
style.css
#results .result {
...
/* display: none; */
}
resultscontroller.js
var results = document.querySelectorAll(".result");
...
results.forEach((result) => result.classList.add("hide"));
The rest of the show/hide situations were handled by replacing the if statements with a single toggle statement, and renaming state to a more meaningful name of shouldShow, andinstead to show/hide the result.
resultscontroller.js
// function toggle(id, state) {
function toggle(result, shouldShow) {
...
// if (state === true) {
// $(result).show();
// }
// if (state === false) {
// $(result).hide()
// }
// if (state === undefined) {
// $(result).toggle(state);
// }
result.classList.toggle("hide", !shouldShow);
}
After getting that confusion cleaned up, the rest of jQuery was easily removed, leaving us with the following code:
resultscontroller.js
/*jslint browser */
import utils from "./utils";
var closeButtonSelector = ".sidebar button";
var clearAllSelector = "#clearAll";
var beforeClose;
function toggle(result, shouldShow) {
var clearAll = document.querySelector(clearAllSelector);
result.classList.toggle("hide", !shouldShow);
var results = document.querySelectorAll(".result");
var hasResults = utils.hasVisible(results);
clearAll.classList.toggle("hide", !hasResults);
}
function getResultId(button) {
var parent = button.parentNode;
while (!parent.classList.contains("result") && parent.nodeName !== "HTML") {
parent = parent.parentNode;
}
return parent.id;
}
function closeClickHandler(evt) {
var button = evt.target;
var id = getResultId(button);
if (typeof beforeClose === "function") {
beforeClose(evt);
}
var result = document.querySelector("#" + id);
result.classList.add("hide");
}
function init(opts) {
beforeClose = opts.beforeClose;
var results = document.querySelectorAll(".result");
var closeButton = document.querySelector(closeButtonSelector);
var clearAll = document.querySelector(clearAllSelector);
results.forEach((result) => result.classList.add("hide"));
closeButton.addEventListener("click", closeClickHandler);
clearAll.classList.add("hide");
clearAll.addEventListener("click", function () {
results.forEach((result) => result.classList.add("hide"));
});
}
export default {
getName: getResultId,
toggle: toggle,
init: init
};
The rest of the files were easy to remove jQuery from without any trouble at all.
Next steps
We’ve removes jQuery from all of the code now.
- Remove jQuery from:
- scrollmanager.js
- scroll.js
- checkboxlist.js
- filtercheckboxesbytext.js
- filtercheckboxesbytext.test.js
- createresults.test.js
- resultscontroller.js
- searchresults.js
- searchresults.test.js
- resultscontroller.test.js
- scrollmanager.js
- Remove global jQuery
- Clear All must show all categories
- Clear All must focus the text entry field
- Use click handler events array
We only need to remove the library from what we’re building and we’ll see great benefits, which I’ll cover next time.
2 Likes
Up until now the webpack’d main script file was nearly a megabyte in size, weighing in at 959 kilobytes.
Version: webpack 4.20.2
Time: 2321ms
Built at: 10/25/2018 9:44:26 PM
Asset Size Chunks Chunk Names
main.css 5.18 KiB main [emitted] main
main.js 959 KiB main [emitted] main
../dist/index.html 1.07 KiB [emitted]
Entrypoint main = main.css main.js
The jQuery library is pretty big. After having removed all uses of jQuery from our code, we can remove the library completely, to gain significant benefits.
Remove jQuery completely
webpack.config.js
// new webpack.ProvidePlugin({
// "$": "jquery",
// "jQuery": "jquery"
// })
> npm remove jquery
and we have now officially rid ourselves of jQuery and its obligations.
We now find the benefit of all the above work. Without jQuery, the full size of our code is only 101 kilobytes, which is close to one tenth of the original size.
Version: webpack 4.20.2
Time: 2861ms
Built at: 10/25/2018 9:45:29 PM
Asset Size Chunks Chunk Names
main.css 5.18 KiB 0 [emitted] main
main.js 101 KiB 0 [emitted] main
../dist/index.html 1.07 KiB [emitted]
Entrypoint main = main.css main.js
While jQuery helped to get us going, without it the code now a lot lighter than it was before, and in many cases is also much more readable too.
Next steps
While removing jQuery I noticed some issues that should next be investigated.
- Remove jQuery
- Remove global jQuery
- Clear All must show all categories
- Clear All must focus the text entry field
- Use click handler events array
I’ll get on to those in the next post.
2 Likes
Fixing clearall issues
While doing these code improvements, I have noticed that there is a small usability problem.
When text entry causes the categories to be filtered, pressing the clear all button clears the text and removes the results, but the list of categories isn’t shown, it’s still filtered.
Another problem is that when the clearAll button is clicked, the focus should go back to the search bar.
Are these issues happening because a test isn’t doing its job, or is it because we’re missing a test for that expected behaviour? Let’s find out.
Investigating clearall tests
The clearall button is managed via the resultcontroller code, and interactions with other parts is handled from the searchresults code, so the searchresult tests are where I should look first.
In the searchresults tests we check that clearall empties the search field, and that it hides any shown results. We aren’t yet testing for what happens to the list of categories, or with the focus, so I’ll start by fixing the focus issue.
Fixing the focus
When the clearall button is clicked, the search field should be given the focus so that what you type immediately ends up there.
First we test, to ensure that we know what we are fixing.
searchresults.test.js
it("gives focus to the search field when clearing all", function () {
clearAllButton.click();
expect(document.activeElement).toBe(categorySearch);
});
That test still passes, so that test cannot tell if we’ve fixed the problem or not.
The reason why it’s not behaving, is that we need to move the focus off the categorysearch element first before doing the testing.
searchresults.test.js
it("gives focus to the search field when clearing all", function () {
var item = categories.querySelector("input");
item.click();
clearAllButton.click();
expect(document.activeElement).toBe(categorySearch);
});
That’s better, the test now correctly fails. Now that we have a failing test, we know that we’re properly checking for the right thing.
Here’s what the code looks like where the existing focus command occurs.
searchresults.js
searchField.value = "";
searchField.focus();
checkboxList.reset();
}
By moving the focus command to the end of the function, the tests go back to all passing
searchresults.js
searchField.value = "";
// searchField.focus();
checkboxList.reset();
searchField.focus();
}
and the focus returns to the search area when clearall is clicked.
Clearall shows all categories
The other thing that needs to be fixed is that clearing all results should leave no categories showing.
First we test, to ensure that we correctly understand the problem that we want to solve:
searchresults.test.js
it("shows all the categories when clearing all", function () {
var items = categories.querySelectorAll("input");
categorySearch.value = "zxc";
utils.keyUp(categorySearch);
expect(utils.getVisible(items).length).toBe(0);
clearAllButton.click();
expect(utils.getVisible(items).length).toBeGreaterThan(0);
});
We can just trigger the keyup event after clearing the search field, to trigger the update.
searchresults.js
function clearAllClickHandler() {
...
searchField.value = "";
utils.keyUp(searchField);
...
}
The clearall button now behaves more like we would expect it to work, and the tests help to ensure that these features continue to remain working.
Next steps
While making these changes I’ve noticed that a mix of var, const, and let are used. It’s long past time to make things more consistent, so converting those needs to be added to our todo list.
I’ve also noticed some objects with duplicate key/value items, which are good candidates for which we can make beneficial use of shorthand properties instead.
- Remove jQuery
- Remove global jQuery
- Clear All must show all categories
- Clear All must focus the text entry field
- Use const/let instead of var
- Use shorthand properties
- Use click handler events array
I’ll carry on to these in the next post.
2 Likes
You and me both, labofoz! All I can say is WOW!
Thank you, Paul. Don’t worry about deciding to get rid of JQuery. I believe the code didn’t work previously due to JQuery (or the correct version of it) not being supported by the software.
1 Like
Last time we noticed that some cleaning up should occur, with the variables, properties, and events.
Turning var to const and let
This was simple as.
Here’s some sample code from beforehand:
scrollmanager.js
function hasScrollContainerAsParent(item) {
var scrollContainer = scroll.getScrollContainer(item);
return scrollContainer === item.parentNode;
}
function upToScrollChild(item) {
var container = item;
...
}
Once JSLint noticed the first const variable, did a great job of letting me know where the remaining var statements were for conversion.
scrollmanager.js
function hasScrollContainerAsParent(item) {
// var scrollContainer = scroll.getScrollContainer(item);
const scrollContainer = scroll.getScrollContainer(item);
return scrollContainer === item.parentNode;
}
function upToScrollChild(item) {
// var container = item;
let container = item;
...
}
Remove duplication from exported methods
Currently the property and method names show a severe amount of duplication, for example:
checkboxlist.js
export default {
uncheckByValue: uncheckByValue,
checkFirst: checkFirstCheckbox,
reset: reset,
init: init
};
We can improve on that using the ES6 property shorthand syntax for method definitions:
checkboxlist.js
export default {
// uncheckByValue: uncheckByValue,
uncheckByValue,
checkFirst: checkFirstCheckbox,
// reset: reset,
reset: reset,
// init: init
init
};
For the sake of consistancy, checkFirstCheckbox should also be renamed to just checkFirst too.
checkboxlist.js
function checkFirstCheckbox() {
function checkFirst() {
...
}
...
export default {
uncheckByValue,
// checkFirst: checkFirstCheckbox,
checkFirst,
reset: reset,
init
};
The same improvements are also made to the other code files too.
Next Steps
- Clear All must show all categories
- Clear All must focus the text entry field
- Use const/let instead of var
- Use shorthand properties
- Use click handler events array
That just leaves the event code to deal with, which I’ll look into next time.
2 Likes
Event handlers
Several techniques are being used for dealing with event handlers, so it’s time to simplify things and make them all consistent with each other.
The most complex situation is where multiple sets of code require using the same event on the same element, so that should be dealt with first. The clearAll click handler is a good one to start with.
Where are clearAll handlers being defined?
It helps to first take an inventory of the code that deals with the clearAll click handler:
In resultscontroller we have the following clearAll code that hides the results:
resultscontroller.js
function clearAllClickHandler() {
const results = document.querySelectorAll(".result");
results.forEach((result) => result.classList.add("hide"));
}
clearAll.addEventListener("click", clearAllClickHandler);
and in searchresults we have the following clearAll code that deals with the search field and checkbox list.
searchresults.js
function clearAllClickHandler() {
const searchField = document.querySelector(searchFieldSelector);
searchField.value = "";
utils.keyUp(searchField);
checkboxList.reset();
searchField.focus();
}
...
clearAll.addEventListener("click", clearAllClickHandler);
Can we use a handlers object?
A simpler way to remove the double-handling is to replace the existing click handler, and call it afterwards. We can do that by making the resultscontroller handler available to other code:
resultscontroller.js
let handlers = {};
...
function clearAllClickHandler() {
...
}
...
function init(opts) {
handlers = {};
...
// clearAll.addEventListener("click", clearAllClickHandler);
handlers.clearallClick = clearAllClickHandler;
clearAll.addEventListener("click", function (evt) {
handlers.clearallClick(evt);
});
...
export default {
handlers,
...
};
We can now place a wrapper around the searchresults clearall handler, so that it does the old event handler along with the new one. That lets us update the resultsController handler.
searchresults.js
function clearAllClickWrapper(oldHandler) {
return function (evt) {
oldHandler(evt);
clearAllClickHandler(evt);
};
}
...
// const clearAll = document.querySelector(clearAllSelector);
// clearAll.addEventListener("click", clearAllClickHandler);
resultsController.handlers.clearallClick = clearAllClickWrapper(
resultsController.handlers.clearallClick
);
On second thoughts
But, we shouldn’t be reaching in to some other module to change things in there. That’s a violation of the open/closed principle that says the code should be open to extension but closed to modification. We are somewhat violating that by modifying the handlers object, so a better technique needs to be used instead.
It would be better if there were instead a clearAll method that lets us add an event handler.
With that in mind we must undo the above changes, and start again with a better approach.
2 Likes
Doing things more properly now, we can improve the event handler by making it capable of being updated, without needing to directly edit anything.
Make handler open for extension
We just need a function method that adds an event handler. That way we can make it available so that other code can add their own clearAll event handlers too.
resultscontroller.js
function addClearallEvent(type, handler) {
const clearAll = document.querySelector(clearAllSelector);
clearAll.addEventListener(type, handler);
}
function init(opts) {
// clearAll.addEventListener("click", clearAllClickHandler);
addClearallEvent("click", clearAllClickHandler);
}
export default {
addClearallEvent,
...
};
We can now easily add another clearall event from the searchresults code, by calling the addClearall method:
searchresults.js
// const clearAll = document.querySelector(clearAllSelector);
// clearAll.addEventListener("click", clearAllClickHandler);
resultsController.addClearallEvent("click", clearAllClickHandler);
While it’s possible to make that addClearallEvent function more generic, and to use a handlers array, and to use a similar foreach technique for the close button events, there’s no good reason to do so yet because no other code wants to add more events onto the close buttons.
Removing the need for afterClick event
Can I use the same technique to improve the checkboxlist events?
checkboxlist.js
function addCheckboxEvent(type, handler) {
const categories = document.querySelector(categoriesSelector);
categories.addEventListener(type, function checkboxFilter(evt) {
const el = evt.target;
if (el.type === "checkbox") {
handler(evt);
}
});
}
...
function init(opts) {
...
addCheckboxEvent("click", clickHandler);
// categories.addEventListener("click", function (evt) {
// const el = evt.target;
// if (el.type === "checkbox") {
// return clickHandler(evt);
// }
// });
...
}
export default {
addCheckboxEvent,
...
};
Things are going well with this. But before further progress can be made, I need to modify the afterClick function so that it accepts the event object, and gets the checkbox from there.
checkboxlist.js
function checkboxClickHandler(evt, afterClick) {
const item = evt.target;
scrollManager.moveTo(item);
afterClick(item);
afterClick(evt);
}
searchresults.js
// afterClick: function (checkbox) {
afterClick: function (evt) {
const checkbox = evt.target;
const result = document.querySelector("#" + checkbox.value);
resultsController.toggle(result, checkbox.checked);
}
I can now add another empty checkbox event, into which we move the above code:
checkboxList.init({
container: categories,
afterClick: function (evt) {
// const checkbox = evt.target;
// const result = document.querySelector("#" + checkbox.value);
// resultsController.toggle(result, checkbox.checked);
}
});
checkboxList.addCheckboxEvent("click", function (evt) {
const checkbox = evt.target;
const result = document.querySelector("#" + checkbox.value);
resultsController.toggle(result, checkbox.checked);
});
And the afterClick code can now be removed:
checkboxlist.js
// function init(opts) {
function init() {
const categories = document.querySelector(categoriesSelector);
// const clickHandler = clickWrapper(opts.afterClick);
// addCheckboxEvent("click", clickHandler);
...
}
searchresults.js
// checkboxList.init({
// afterClick: function (evt) {
// }
// });
checkboxList.init();
Making code open for extension but closed for modification, has resulted in code that is easier to understand and harder to break.
2 Likes
Removing the need for beforeClose event
In the above code I removed the need for the afterClick code. I should be able to achieve a similar benefit by removing the beforeClose code too.
Testing separate events
First, I check to see if the events can be defined separately:
resultscontroller.js
function init(opts) {
...
// addClosebuttonsEvent("click", closeClickWrapper(opts.beforeClose));
addClosebuttonsEvent("click", closeClickHandler);
addClosebuttonsEvent("click", opts.beforeClose);
...
}
export default {
addClosebuttonsEvent,
...
};
Replace beforeClose events
The beforeClose linenow can be replaced by a similar one in the searchresults code:
searchresults.js
// resultsController.init({
// beforeClose: uncheckCheckboxHandler
// });
resultsController.init();
resultsController.addClosebuttonsEvent("click", uncheckCheckboxHandler);
resultscontroller.js
// function init(opts) {
function init() {
...
addClosebuttonsEvent("click", closeClickHandler);
// addClosebuttonsEvent("click", opts.beforeClose);
...
}
Remove beforeClose code
And now, we can remove the beforeClose complexity from the code:
// function closeClickHandler(evt, beforeClose) {
function closeClickHandler(evt) {
const button = evt.target;
const id = getName(button);
const result = document.querySelector("#" + id);
result.classList.add("hide");
// if (typeof beforeClose === "function") {
// beforeClose(evt);
// }
}
// function closeClickWrapper(beforeClose) {
// return function clickHandler(evt) {
// closeClickHandler(evt, beforeClose);
// };
// }
With these simple changes, we are gradually cleaning up and improving the code. Later on we might need to add proper beforeClose event handlers, but that’s something for later, and is not a complexity that needed just as of yet.
Next steps
There’s always more improvements that can be done to the code, but there doesn’t seem to be much of a concerning nature remaining.
Next time we’ll take a look at some organisation that needs to be done with code files.
2 Likes
Finishing things up
Now that everything’s been done to update the code, we can look at how people will be adding it to their own project.
The main filenames really should be changed to searchresults instead, to make it easier for people to use it with their own code.
Move the JavaScript file
webpack.config.js
output: {
filename: "js/[name].js"
},
Move the CSS file
webpack.config.js
new MiniCssExtractPlugin({
// filename: "[name].css",
filename: "css/[name].css",
// chunkFilename: "[id].css"
chunkFilename: "css/[id].css"
}),
Rename the js and css files
Because we’re using [name] for both the js and css sections, we can now easily rename them from the default of main. It’s fairly easy to rename it to searchresults just by specifying an entry point.
webpack.config.js
entry: {
"searchresults": "./src/index.js"
},
Pack the CSS code
Currently the CSS code is unpacked. As we’re using webpack for JavaScript, it would be nice if we could pack up the CSS code too. Fortunately there is the OptimizeCSSAssetsPlugin plugin.
webpack.config.js
const OptimizeCSSAssetsPlugin = require("optimize-css-assets-webpack-plugin");
And because this means updating the optimizer settings, we need to add in the previously unstated default JavaScript optimization code too. Big thanks to Minimising for Production for the details on how to achieve this.
webpack.config.js
optimization: {
minimizer: [
new UglifyJsPlugin({
cache: true,
parallel: true,
sourceMap: true
}),
new OptimizeCSSAssetsPlugin({})
]
},
and we can now add OptimizeCSSAssetsPlugin to the plugins
plugins: [
...
new OptimizeCSSAssetsPlugin({
assetNameRegExp: /\.optimize\.css$/g,
cssProcessor: require("cssnano"),
cssProcessorPluginOptions: {
preset: ["default", {
discardComments: {removeAll: true}
}]
},
canPrint: true
})
],
Next steps
Things seem to be all ready now. Next time I’ll put the code up on github and do some checks to ensure that everything is ready to be used.
2 Likes
Thank you for your wonderful work yet again, Paul!
While doing the above code improvements, I should have been adding each update to a git repository. It’s now time to get git going, so that I can add the existing code there and easily manage change from now on.
Create a repository
First I go to github.com and create a repository there with an MIT license, which is about the most open license that I know of.
When it comes to git commands, I find that the GIT guide is an easy to access reference.
Then from the command prompt I can add this coding project to the online repository.
Instead of the full pathname of C:\Users\yourname\search-results> in the below extracts, I’ll just use the > carat symbol instead.
>git init
Initialized empty Git repository in C:/Users/yourname/search-results/.git/
And I can tell the local repository where to find the remote one.
>git remote add origin https://github.com/pmw57/search-results
Use a Develop branch
I don’t want to push this content to master just yet, as I don’t think that it’s fully ready.
Instead, I’ll create a branch called develop, and use that branch instead.
>git checkout -b develop
Switched to a new branch 'develop'
When I have the code and the documentation in a state that I consider ready to be used, I’ll then use the Master branch for that.
Add files
I can now use git status to view untracked files and folders
>git status
On branch develop
No commits yet
Untracked files:
(use "git add <file>..." to include in what will be committed)
.babelrc
.gitignore
config/
css/
dist/
install.txt
karma.conf.js
node_modules/
package-lock.json
package.json
src/
test/
webpack.config.js
nothing added to commit but untracked files present (use "git add" to track)
I don’t want the node_modules folder to be tracked by git, as that can be easily generated afterwards. I add node_modules to a .gitignore file
.gitignore
node_modules
git status now shows the same list without node_modules
I’m okay now with adding all of those files to the develop branch
>git add *
warning: LF will be replaced by CRLF in dist/js/searchresults.js.
The file will have its original line endings in your working directory.
warning: LF will be replaced by CRLF in karma.conf.js.
The file will have its original line endings in your working directory.
warning: LF will be replaced by CRLF in package-lock.json.
The file will have its original line endings in your working directory.
warning: LF will be replaced by CRLF in package.json.
The file will have its original line endings in your working directory.
And I can now commit all of those added files to my local develop branch
>git commit -m "Initial commit"
[develop (root-commit) df433b7] Initial commit
37 files changed, 12505 insertions(+)
create mode 100644 .babelrc
create mode 100644 .gitignore
create mode 100644 config/categories.json
create mode 100644 css/style.css
create mode 100644 dist/css/searchresults.css
create mode 100644 dist/index.html
create mode 100644 dist/js/searchresults.js
create mode 100644 install.txt
create mode 100644 karma.conf.js
create mode 100644 package-lock.json
create mode 100644 package.json
create mode 100644 post.txt
create mode 100644 src/checkboxlist.js
create mode 100644 src/createcategories.js
create mode 100644 src/createresults.js
create mode 100644 src/filtercheckboxesbytext.js
create mode 100644 src/index.js
create mode 100644 src/resultscontroller.js
create mode 100644 src/scroll.js
create mode 100644 src/scrollmanager.js
create mode 100644 src/searchresults.js
create mode 100644 src/static/index.html
create mode 100644 src/utils.js
create mode 100644 test/categories.json
create mode 100644 test/checkboxlist.test.js
create mode 100644 test/create-sandbox.js
create mode 100644 test/create-sandbox.test.js
create mode 100644 test/createcategories.test.js
create mode 100644 test/createresults.test.js
create mode 100644 test/filtercheckboxesbytext.test.js
create mode 100644 test/resultscontroller.test.js
create mode 100644 test/sandbox.js
create mode 100644 test/sandbox.test.js
create mode 100644 test/scroll.test.js
create mode 100644 test/scrollmanager.test.js
create mode 100644 test/searchresults.test.js
create mode 100644 webpack.config.js
Pushing files upstream
Pushing the committed content up to the server comes next, but there’s a problem:
>git push
fatal: The current branch develop has no upstream branch.
To push the current branch and set the remote as upstream, use
git push --set-upstream origin develop
I need to tell git to which destination branch to push things.
With the entry of my github username and password, things are successfully pushed.
>git push --set-upstream origin develop
fatal: HttpRequestException encountered.
An error occurred while sending the request.
Username for 'https://github.com': [elided]
Password for 'https://[elided]@github.com':
Counting objects: 46, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (40/40), done.
Writing objects: 100% (46/46), 136.38 KiB | 4.55 MiB/s, done.
Total 46 (delta 2), reused 0 (delta 0)
remote: Resolving deltas: 100% (2/2), done.
remote:
remote: Create a pull request for 'develop' on GitHub by visiting:
remote: https://github.com/pmw57/search-results/pull/new/develop
remote:
To https://github.com/pmw57/search-results
* [new branch] develop -> develop
Branch develop set up to track remote branch develop from origin.
Fix a few issues
Remove the HttpRequestException
I should also fix that HttpRequestException that occurred. That’s because Git has updated to TLS-1.2 and removed TLS-1.0 so I need to download and install an updated git program from https://git-scm.com/download/win
My existing git is at version 2.14.0-windows.2
After installing the update, it’s now at version 2.21.0-windows.1 and git push no longer shows the exception.
Remember username and password
If I feel assured of my local computer security, I can tell git to remember my username and password too.
>git config --global credential.helper store
When I do a git push now, this should be the last time that I need to enter my credentials
>git push
Username for 'https://github.com': [elided]
Password for 'https://[elided]@github.com':
Everything up-to-date
Future git push commands now just work.
>git push
Everything up-to-date
Next steps
I can now work on making the code easily available for other people to use, where I’ll start with a blank HTML page and write some documentation about what needs to be done to get things working.
That will start with a fresh clone of the github repository, which I’ll get in to in the next post.
2 Likes