Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move from jQuery to using Vanilla JS (ES5) #163

Open
wants to merge 46 commits into
base: master
from

Conversation

@fabiankaegy
Copy link
Contributor

commented Sep 9, 2019

After our discussion today in the core-themes channel in slack I took the work done in #101 and refactored it to only use es5 syntax so there is no need for build tooling.

I still very much think that the other approach might be more maintainable but can see why build tooling might scare many away. So this should be all the work done over there without using any build tooling or modern js.

assets/js/index.js Outdated Show resolved Hide resolved
@@ -154,9 +154,9 @@ function twentytwenty_register_scripts() {
wp_enqueue_script( 'comment-reply' );
}
$js_dependencies = array( 'jquery' );
$js_dependencies = array( 'wp-polyfill', 'wp-dom-ready' );

This comment has been minimized.

Copy link
@acosmin

acosmin Sep 10, 2019

Contributor

I might be wrong but polyfill and dom-ready are available only in WP v5.*, everything below that will cause issues.

assets/js/index.js Outdated Show resolved Hide resolved
// On-page links
if (window.location.hostname === event.target.hostname) {
// Figure out element to scroll to
debugger;

This comment has been minimized.

Copy link
@acosmin

acosmin Sep 10, 2019

Contributor

This should be removed and to make the event.target.hostname work we should use some CSS on the specific buttons, that's if we don't want go more deep and add some parentNodes in the mix. The simple solution would be (eg: for back to top button):

a.to-the-top > * {
    pointer-events: none;
}
Interval Scroll
--------------------------------------------------------------------------------------------------- */

twentytwenty.intervalScroll = {

This comment has been minimized.

Copy link
@acosmin

acosmin Sep 16, 2019

Contributor

I think we can get rid of this function, the event isn't used anywhere...

This comment has been minimized.

Copy link
@acosmin

acosmin Sep 16, 2019

Contributor

same for twentytwenty.resizeEnd

This comment has been minimized.

Copy link
@fabiankaegy

fabiankaegy Sep 16, 2019

Author Contributor

agreed 👍 this far I only mirrored all the functionality from the original file. I did not jet take unused stuff out. Wasn't sure wether that should happen as part of the same PR.

@acosmin

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@pattonwebz I think it's ready for merge :)

Maybe you can get someone else to take a look.

We could've probably made it compatible with WP versions under 5.0... the dom-ready dep can be written in 2-3 lines, as for polyfill :) I can't say how many of those are used...

@fabiankaegy great job on this!

@fabiankaegy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@acosmin cool thank you. I'm not opposed to replacing dom-ready with our own function. Just wasn't sure wether we really need write it again and again when it's already there. But yes it is only in 5.0 and up...

as for the polyfill I am not certain myself maybe someone who is a bit more familiar with the package can say more about wether they think it really gives us a lot in regards to older browsers.

As far as I know the only thing that the polyfill is currently needed for is Object.assign() but again I'm far from being an expert on polyfills :)

@dingo-d

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

One thing that I'm on the fence about is that all the JS code is in one file (it's 900+ loc). Could a component based approach be used here? So that all the individual parts (coverModals, intrinsicRatioVideos, intervalScroll etc.) are in their files as modules, and then the init is done in an index.js file using ES5 require?

This is what I'm doing (to an extent) in the theme sniffer. The index.js file is where I define the parts which interact with the DOM, and the functionality is in the other class file. Granted I'm using ES6 because of the build tools, and I need to further extract certain functionality, but the principle is there.

@fabiankaegy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@dingo-d Hi there, yeah I first went with that approach, but then we decided in the weekly chat last week that there should be no build process and therefore I removed all ES6. The first approach was #101

@andersnoren

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@fabiankaegy First, huge thanks for taking the lead on this one – it's been a big relief for us that you've been able to put in so much work on it.

I thought I'd weigh in on what code I think can be removed altogether from the JavaScript file (since I'm responsible for the original jQuery code).

I believe these ones can be safely removed altogether:

twentytwenty.intervalScroll.init();			// Check for scroll on an interval
twentytwenty.dynamicScreenHeight.init();	// Dynamic Screen Height
twentytwenty.resizeEnd.init();				// Trigger event at end of resize

Since all of the modal toggles are in the top of the page, the scroll lock function can be rewritten to just set the scrollTop position to 0 and the body to overflow: hidden. This will also fix an issue we have with the admin bar position when the scrollLock (as it works currently) is active.

twentytwenty.scrollLock.init();				// Scroll Lock

I think we need some variation of the rest, but they can all be simplified significantly to be more specific to the elements in Twenty Twenty.

There are also additions needed (handling focus for the horizontal menu, for example), but it might be better to make those changes once this PR is merged.

@dingo-d

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

@dingo-d Hi there, yeah I first went with that approach, but then we decided in the weekly chat last week that there should be no build process and therefore I removed all ES6. The first approach was #101

Oh so not even using basic require.js, that's sad :/

fabiankaegy and others added 6 commits Sep 16, 2019

@carolinan carolinan moved this from To do to In progress in Beta 1 Sep 17, 2019

@andersnoren

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

@fabiankaegy I did some testing on Mac (and in Browserstack) this morning:

On IE11, I get the following error:

SCRIPT438: Object doesn't support property or method 'forEach’, index.js (484, 3)

When the ”Expanded Menu” and ”Mobile Menu” have the same menu set, the sub nav toggles doesn’t work on mobile. When the menu locations are set to different menus (in which case two separate menu elements are output), they work on both mobile and desktop.

Clicking the mobile menu toggle rapidly can cause the screen to be locked, but the menu to be hidden (on Safari and Chrome). Same thing seems to happen with search on mobile and desktop, but not with the menu toggle on desktop.

@ianbelanger79

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

@fabiankaegy Just tested the PR on Windows 10 in Firefox, Chrome, Edge and IE11.

Everything looks good, except the modals do not work in IE11. Other than that, I think this is in good shape.

ianbelanger79 and others added 5 commits Sep 17, 2019
@@ -207,7 +207,8 @@ function twentytwenty_register_scripts() {
$js_dependencies = array( 'wp-dom-ready', 'wp-polyfill' );
wp_enqueue_script( 'twentytwenty-index', get_template_directory_uri() . '/assets/js/index.js', $js_dependencies, $theme_version, false );
wp_enqueue_script( 'twentytwenty-construct', get_template_directory_uri() . '/assets/js/construct.js', $js_dependencies, $theme_version, false );

This comment has been minimized.

Copy link
@fabiankaegy

fabiankaegy Sep 18, 2019

Author Contributor

This is now pointing to the wrong file. Should I rename the index.js file to construct.js again?

This comment has been minimized.

Copy link
@ianbelanger79

ianbelanger79 Sep 18, 2019

Contributor

No I will fix it, I prefer index.js

@ianbelanger79

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Unfortunately this still does not work in IE11


// Trigger events on the toggle targets before they are toggled
if (target.classList.contains('active')) {
target.dispatchEvent(new Event('toggle-target-before-active'));

This comment has been minimized.

Copy link
@acosmin

acosmin Sep 18, 2019

Contributor

Seems like Event() isn't supported by IE

https://developer.mozilla.org/en-US/docs/Web/API/Event/Event

😞

This comment has been minimized.

Copy link
@acosmin

acosmin Sep 18, 2019

Contributor

I've tested this polyfill and the menu and search modal work as intended...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.