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

Remove JavaScript detection using class names #2916

Merged
merged 1 commit into from Jan 2, 2022
Merged

Conversation

knowler
Copy link
Member

@knowler knowler commented Dec 29, 2021

This PR took a bit of a turn. Originally, it moved existing JavaScript detection from the body to the html element as well as the code to do that to the index.php file. After some feedback and further research, I began to question the necessity of the JavaScript detection in the first place. I struggled to find websites that actually used JavaScript detection with a class name on html or body and when they did there was no code on the site that actually used the class name for any non-JS or enhanced effect.

I’m convinced that this method has lost favour to progressive enhancement methods which emphasize writing styles that work for HTML out of the box. In these methods, increased specificity (e.g. the targeting the presence of attributes) is used to style components when they are enhanced with JavaScript. An example of this in practice might be web components that dynamically import and adopt their stylesheet upon registration. The philosophy here is to allow browsers that want to work less, to do so instead of working more (e.g. writing .no-js styles that override fully enhanced component styles).

@github-actions github-actions bot added javascript Pull requests that update Javascript code views labels Dec 29, 2021
@retlehs retlehs added this to the 10.0.0-beta.3 milestone Dec 29, 2021
@knowler
Copy link
Member Author

knowler commented Dec 29, 2021

I have not tested this. The only thing I think which could go wrong is maybe needing to add WordPress’ dom-ready to the deps array for the editor script. I don’t know where that is though.

Also, adding the no-js removal inline script in index.php might be non-standard (as opposed to using wp_head), but I figure it’s close to where the class name is set on the element and therefore easier to see what it’s doing.

@knowler
Copy link
Member Author

knowler commented Dec 29, 2021

Ok, I’ve tested this and it works. There’s no need to add any deps for the editor script since wp-dom-ready is in the window. I also just added a js class after the no-js one gets removed. Might be helpful and its in the link I referenced in the PR description. Not sure why PHP 7.3 is failing, but I think it was like that before.

@knowler knowler self-assigned this Dec 29, 2021
Copy link
Sponsor Member

@retlehs retlehs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we keep a domReady example or starting point in app.js?

@retlehs
Copy link
Sponsor Member

retlehs commented Dec 29, 2021

Not sure why PHP 7.3 is failing, but I think it was like that before.

latest acorn release is triggering those failures rn, you can ignore (we're gonna just remove php 7.3 support)

@knowler
Copy link
Member Author

knowler commented Dec 29, 2021

could we keep a domReady example or starting point in app.js?

We can do that. We would need to add our domReady script back though.

@Log1x
Copy link
Sponsor Member

Log1x commented Dec 29, 2021

We can do that. We would need to add our domReady script back though.

I vote we add it back and keep it how it was outside of moving no-js to the html element.

@knowler
Copy link
Member Author

knowler commented Dec 29, 2021

I added it back. Should we add it back to editor.js too? Or is using @wordpress/dom-ready there fine (since it’s already available in the admin).

JavaScript detection with a class name on the body or html element tends
to go unused even by websites that have it set up. Instead, we recommend
that developers write progressively enhanced components that are styled
for non-JS usage by default. Increase specificity, use the cascade, or
code split to apply progressively enhanced styles.
@knowler
Copy link
Member Author

knowler commented Dec 30, 2021

I changed directions here:

  • This now removes the JS detection (see updated PR description for reasoning).
  • It doesn’t change anything with the domReady stuff like it originally did.

@knowler knowler changed the title Set no-js on HTML element and remove earlier Remove JavaScript detection Dec 30, 2021
@knowler knowler changed the title Remove JavaScript detection Remove JavaScript detection using class names Dec 30, 2021
@retlehs retlehs merged commit 6523ebc into main Jan 2, 2022
@retlehs retlehs deleted the no-js-on-html branch January 2, 2022 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code views
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants