Opened 10 years ago
Closed 10 years ago
#30056 closed defect (bug) (fixed)
Twenty Fifteen: Menu hiccups
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 4.1 | Priority: | normal |
| Severity: | normal | Version: | 4.1 |
| Component: | Bundled Theme | Keywords: | has-patch commit |
| Focuses: | ui, javascript | Cc: |
Description
The no-js class is triggering the sub menus to show for a brief period of time until JS removes that class. I couldn't find the another best alternative to this but it feels a bit annoying when you have that brief flashing of sub menus and collapsing. The scroll bar appears for a brief period if the menu/sidebar is long.
Attachments (4)
Change History (19)
#2
@
10 years ago
- Component changed from General to Bundled Theme
- Summary changed from Menu hiccups to Twenty Fifteen: Menu hiccups
#4
@
10 years ago
- Keywords has-patch needs-testing added; needs-patch removed
What about doing the same js check with non-jQuery way? We will need to have one more file but if we link it inside <head> with wp_enqueue_script(), we don't see the flash anymore. Does it worth it?
#5
@
10 years ago
It's generally accepted (ref) to do this inline in the document's <head> so that it takes effect instantly before anything is loaded. 30056.diff actually makes things worse by loading an extra javascript file, slowing down the site load.
Patch incoming for the inline approach.
#6
@
10 years ago
The only reason I enqueued was that I've been told all scripts have to be linked with wp_enqueue_script() before, but it makes more sense to make this an exception.
This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.
10 years ago
#9
@
10 years ago
Worth noting that in 2011 Paul Irish, by suggestion of Dan Beam, changed that regular expression that used word boundaries with a new one to avoid some (rare) edge cases, namely \b also matches - or +.
For example, it fails if there's already a class like, say, myplugin-no-js as first class.
The change is here:
https://github.com/Modernizr/Modernizr/pull/358/files
(/(^|\s)no-js(\s|$)/, '$1$2')
For reference, it was changed again here:
https://github.com/Modernizr/Modernizr/commit/71894c33b00082a52a9474a07751b3290576abf7
to add the ability to prefix that class, not useful in our case.
Current version source:
https://github.com/Modernizr/Modernizr/blob/master/src/setClasses.js
@
10 years ago
Updated patch with new regex, rebuilt from the root. Would like a second opinion though.
#11
@
10 years ago
The updated patch is fine, but really unnecessary since it runs before wp_head() (and therefore any 3rd-party scripts) and we have the no-js document class hardcoded. We don't need to handle edge cases that won't happen. :)
#12
@
10 years ago
Nice point @mattwiebe, you're right :) So if your assumption is correct, we don't need to match word boundaries either. Just a simple replace "no-js" with "" ?
Btw I would be more careful, assumptions are the root of all evil :) even if the only thing that comes to my mind now is what child themes could do hardcoding additional CSS classes. Should we take that into account?
Oh, Can't edit the ticket. This is Twenty Fifteen theme.