Skip to:
Content

BuddyPress.org

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#7314 closed defect (bug) (fixed)

Buddypress issue with Twenty Sixteen menu

Reported by: darkwolf's profile DarkWolf Owned by: hnla's profile hnla
Milestone: 2.7.3 Priority: normal
Severity: normal Version: 2.7
Component: Templates Keywords: dev-feedback commit
Cc: salvatore.noschese@…

Description

I've report here some months ago:
https://buddypress.org/support/topic/issue-with-sixteen-mobile-menu-and-buddypress/
and here (with a provisory fix):
https://wordpress.org/support/topic/sixteen-menu-is-open-on-pageload/?replies=6#post-8575130

I've try again just now with new install in local webserver:

1) download wordpress (ita) and put in wamp directory;
2) run wamp, open localhost, open localhost/wordpress and install (new database etc);
3) install buddypress;
4) setup some item in twenty sixteen menu and test!

issue is present. no problem with others wordpress default theme (so, is just an issue with sixteen).

here a provisory fix, but need more investigation:
https://wordpress.org/support/topic/sixteen-menu-is-open-on-pageload/#post-8371032

hope this info can help (and sorry for poor english).

regards,
S.N.

Attachments (5)

buddybug.jpg (66.8 KB) - added by DarkWolf 7 years ago.
menu issue during pageload
buddyok.JPG (70.2 KB) - added by DarkWolf 7 years ago.
ok after some seconds…
7314.gif (61.2 KB) - added by slaFFik 7 years ago.
7314-css-workaround.patch (1.2 KB) - added by hnla 7 years ago.
7314-css-fix-commented.patch (1.9 KB) - added by hnla 7 years ago.
Update patch with inline comments for clarity.

Download all attachments as: .zip

Change History (30)

@DarkWolf
7 years ago

menu issue during pageload

@DarkWolf
7 years ago

ok after some seconds...

#1 @DarkWolf
7 years ago

  • Cc salvatore.noschese@… added
  • Component changed from Core to Templates

just to ble more clear: i've this issue only in mobile mode (then, also if i reduce firefox screen) and is really "unpleasant" with mobile device :/

regads,
S.N.

#2 @DJPaul
7 years ago

  • Keywords reporter-feedback added

@DarkWolf What is the *actual* problem? You've described how to test well, but when I do so, I can't see what's wrong. How is it behaving that you think it shouldn't?

#3 @DarkWolf
7 years ago

The problem is:
With buddypress, during pageload, menu - in mobile mode view - is expanded and turn back ok (just a button) only when pageload is complete.
Without buddypress, instead, menu is never expandend (nor during pageload, nor when pageload is complete) and work as expected (is open only onclick).

Thanks for reply.

Regards,
S.N.

#4 @r-a-y
7 years ago

  • Keywords dev-feedback added; reporter-feedback removed

@DarkWolf notes that the problem is with the addition of the no-js <body> CSS class for bp-legacy:
https://wordpress.org/support/topic/sixteen-menu-is-open-on-pageload/#post-7516921

This conflicts with Twenty Sixteen, which also has its own no-js detection method:
https://github.com/WordPress/twentysixteen/issues/408

Not sure what we should do here. Do we do a conditional to see if Twenty Sixteen is loaded and if so, do not add our custom no-js CSS class?

Last edited 7 years ago by r-a-y (previous) (diff)

#5 @hnla
7 years ago

@r-a-y if we start doing checks like this - and I've thought often of doing so - we accept a increasing burden on fine tuning for given themes, if we do so here how many other instances of things that we could manipulate checking what theme is selected and changing code/rules to accommodate do we start doing? ( not saying we don't though).

At this moment checking and I can't replicate this issue though! Browser specific?

Also 2016 isn't the only theme to hardcode the class to the html element 2015 does too.

Last edited 7 years ago by hnla (previous) (diff)

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


7 years ago

#7 @DarkWolf
7 years ago

I've test with Firefox and chrome + Lumia device (iemobile) and issue is present. Then, is an issue with no-js from buddypress with a standard wp theme and, imho, this need,to be fixed (if xyz* cause issue with default wp theme... xyz* need to be fixed).

I've this issue, in my dev live website: laltroweb.it/_dev
+ in localhost clean install (just buddypress and WordPress with some element in menu).
Disable buddypress or remove no-js class from buddypress and all is good.
Clearly, this class cause an issue with default WordPress theme (and I really hope this will be considered a bug - otherwise, sorry, this bug report is unuseful).

Regards,
S.N.

#8 @hnla
7 years ago

Well it wouldn't hurt to have a check available in buddypress-functions.php or core somewhere for that matter for active wp theme.

As for removing classes, my only worry is if styles had been hooked to it.

2015, 2016, 2017 all have this class on the html element fwiw.

@r-a-y lets add a check then? my crude approach $this->wp_theme = strtolower( str_replace( ' ', '', wp_get_theme() ) ); and I guess an array of twenty* themes with that class to check against.

#9 @DJPaul
7 years ago

  • Milestone changed from Awaiting Review to 2.7.3

#10 @DJPaul
7 years ago

I think that is the wrong approach.

--> 2017 has no-js in its template, and removes it if JS is present in its twentyseventeen_javascript_detection() (hooked to wp_head at priority 0), which will run pretty soon after page load. It's VERY near the top of the page.

--> BP adds no-js via body_class and removes it in our buddypress.js, inside a jQuery ready callback. (i.e. much later than 2017's).

I don't understand really what the problem is. Even if the theme temporarily thought it had no JS support, it ought to "flash back" with JS features appearing, after DOM Ready... right? I don't understand why this isn't happening. I'm going to assume it's because of some quirk of load order and timing. (confused!)

At any rate, we could basically just copy/paste the JS in twentyseventeen_javascript_detection() into the VERY top of our buddypress.js, which means that our no-js will be removed much quicker than it is. Can someone who knows how to recreate this problem, test the approach? Or maybe talk me through it on Slack sometime so I can learn.

Last edited 7 years ago by DJPaul (previous) (diff)

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


7 years ago

@slaFFik
7 years ago

#12 @hnla
7 years ago

Can we test this workaround css patch.
it attempts to hide the siteheader menu if html.js and body.no-js on the basis that we cut in later with our remove .no-js so onclick of a link and page load the effective state of the body class should be no-js and thse new rules take effect to hide the elements until full page load.

It feels as though it works or at least improves things at a throttled mobile view but equally could be mis-reading result and the rule will not be loaded until we hit the body element so likely isn't really an improvement:(

Last edited 7 years ago by hnla (previous) (diff)

#13 @DJPaul
7 years ago

i'll give it a spin after wok, thanks for exploring that idea

#14 @DJPaul
7 years ago

  • Keywords commit added

@hnla your CSS patch looks good and works well. Go for it!

#15 @hnla
7 years ago

@djpaul cool I'll attend to in the morning.

#16 @hnla
7 years ago

  • Owner set to hnla
  • Status changed from new to assigned

#17 @hnla
7 years ago

Update patch to include comments for scss/css files ruleset - detail issue for scss, brief note for css.

@hnla
7 years ago

Update patch with inline comments for clarity.

#18 @hnla
7 years ago

In 11263:

Correct twentysixteen .no-js class conflict with BP's version of same class on body element

Commit adds a ruleset specific to twntysixteen to hide the onclick dropdown mobile menu on page load.

Twentysixteen adds a hardcoded class to the html element & changes class if JS active scripted in the 'head'; BP runs similar but on body el & changes no-js class on DOM fully loaded.

Menu remains in view after clicking due to 2016 updating js class early and BP lagging behind.

Ruleset targets body element if it has a class .no-js and it's parent has class .js thus we only run if JS active and for initial page load sequence.

See #7314

#19 @hnla
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 11264:

Correct twentysixteen .no-js class conflict with BP's version of same class on body element

Commit adds a ruleset specific to twntysixteen to hide the onclick dropdown mobile menu on page load.

Twentysixteen adds a hardcoded class to the html element & changes class if JS active scripted in the 'head'; BP runs similar but on body el & changes no-js class on DOM fully loaded.

Menu remains in view after clicking due to 2016 updating js class early and BP lagging behind.

Ruleset targets body element if it has a class .no-js and it's parent has class .js thus we only run if JS active and for initial page load sequence.

Fixes #7314 ( Branch 2.7 )
Props DarkWolf, DJPaul, r-a-y, hnla

#20 @hnla
7 years ago

@DarkWolf We have looked at this from a variety of angles to try and see what potential fixes would work, our big problem is in the point at which our scripts cut in to change the class tokens & this isn't an issue we can easily resolve.

We have settled on an approach to reduce the issue by ensuring that the menu elements are hidden if 2016 class is .js (thus JS active) and our body class reads as .no-js ( our script hasn't yet been able to run to change to .js this should help to reduce the noticeable effect on these slower connections.

#21 @DarkWolf
7 years ago

ok, thank'u, i'll test as soon as possible your css patch and release a feedback

regards :)

#22 @DarkWolf
7 years ago

edit: is necessary to edit also twentysixteen.min.css

search:

@charset "UTF-8";

replace:

@charset "UTF-8";@media screen and (max-width:905px){html.js body.no-js .site-header-menu{display:none}}

after of this, all seems to work as expected :)

Last edited 7 years ago by DarkWolf (previous) (diff)

#23 @DJPaul
7 years ago

That file will get rebuilt when we package the next release.

#24 @hnla
7 years ago

@DarkWolf As DJPaul says - those files are part of the grunt 'build' task and not run for initial commits to the /src/ directory.

Thanks for testing the patch though, hope this helps the immediate issue.

#25 @DarkWolf
7 years ago

Ok, thank'u too for check on this issue and found this fix.

best regards,
S.N.

Note: See TracTickets for help on using tickets.