Closed michaelbourne closed 1 year ago
height[bot]
commented
1 year ago Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword
link(e.g. "Link T-123").💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.
ronilaukkarinen
commented
1 year ago Thanks a lot for the effort @michaelbourne! I'll test it out next week. 👍
ronilaukkarinen
commented
1 year ago @michaelbourne Had a quick test. Are you sure this is right? I fetched your changes and rebased them with the current master. Console log shows now errors, but the nav looks like this, even after rebuilding JS and CSS:
I suspect the .js class is the cause, because we no longer use no-js/js at all. In some point we decided to stop supporting browsers without JavaScript for the reasons we stopped IE support - the market share for them is so small.
Can you re-add js/no-js to this PR?
Also could you make sure that the JS build succeedes. This means using our .eslintrc (VSCode has an autoformat plugin for Eslint as well) or have our packages installed with npm/npx and to test with eslint . in the theme directory. Thanks!
michaelbourne
commented
1 year ago Hi @ronilaukkarinen
Good catch, I still use the js and no-js classes. Not really to support JS disabled browsers explicitly, but for things like this nav change. My browserslist is actually more limited than yours now due to the types of sites I'm building, I don't have any old browser support.
If you're 100% adamant against having it, there's probably another angle of attack here. I just know something needed tweaking to remove the initial CLS of those nav menus.
michaelbourne
commented
1 year ago Tested that build on a new install and it seems to work as intended. You can test the progressive enhancement by disabling JS.
When we have sites audits by 3rd party vendors like https://accessibleweb.com/ , having a working menu with JS disabled is a criteria they check for. It's obviously an enhanced qualification, not your basic AA level, but for the little effort it takes, seems worthwhile. Not sure it would be possible (cleanly) without the js classes but we can talk bout it.
A few left over items I noticed in testing. There was always a flash from submenus on page load (CLS) and the mobile menu. There was an issue with resized windows (especially with dev tools responsive mode) and the submenu out of viewport measurement. Mobile menu was wider than screen at some device sizes. This fixes all of that. Also removed erroneous nav toggle height in the JS since it was no longer used by the CSS.
Opinionated: I moved some styles around and prefixed with
.jsand.no-jsfor a little better control over how the desktop menu works. Whilst having JS disabled is not conducive to accessibility, I still wanted a functional menu. This PR achieves that.