search

CU-CommunityApps / cwd_base

A lightweight Drupal 8+ theme, implementing Cornell Branding and standard CWD components.
1 stars 2 forks source link

section nav mobile menu markup causing sidebar nav to be shown #29

Open alisonjo315 opened 4 years ago

alisonjo315 commented 4 years ago

Note: This issue maybe can be dropped in favor of #58 (and with work done on other issues, as detailed in the thread on this issue).

Original issue description:

(mayyyybe from this commit?)

Part of how empty sidebars are hidden is via the "empty" class that's added via cwd_variables.js, when #sidebar-top is empty. BUT, at some point, this class stopped getting applied, maybe/probably because the element isn't empty anymore, right?

I'm not sure if it's b/c of recent(ish) JS updates for mobile expander things, or if it's b/c of something else, but, the sidebar block isn't getting .empty anymore: https://dev-cd-demo.pantheonsite.io/test-page-without-sidebar

(Compared with a site that has a much older version of cwd_base, FCS -- has cwd_base 1.0.0-alpha22 -- example page with no section navigation content -- hopefully the link will still work by the time "you" click)

ama39 commented 4 years ago

There are two pieces of information that might help:

  1. The .empty class script may not detect the region as empty if Twig Debugging is on, since it renders markup (comments) and whitespace inside the region. If you can, try turning off debugging and see if it helps.

  2. The block region is not naturally empty on this cd-demo page. Even though the page is not in the menu structure, the section navigation block is still there and produces a menu with the top level links for the site (Home, Custom Development, News, etc...). I use CSS to hide these top level links and only show the active trail (treating the top level link in the trail as a section title). Additionally, JavaScript checks if the menu has no links beyond the section title. If this is the case, instead of rendering the section title alone, it just removes the whole menu from the DOM. That's why the block looks empty even if you inspect the source output.

So it's possible item 1 will solve the problem. If not, it's possible the empty menu removal is happening too late and needs to run before the script that generates a mobile expander, so that the mobile expander script skips it.

alisonjo315 commented 4 years ago

Ugh, so annoying when my demo content disappears...

Debugging isn't affecting things -- here's an example on FCS (which has an older version of cwd_base) where the full-width layout / .empty class is working fine: https://dev-fcs-main.pantheonsite.io/test-page-not-menu

(I'll update the issue description with this new link.)

alisonjo315 commented 4 years ago

Hmm I guess you're saying that maybe debugging is affecting things on pages with the newer version of the theme... I can try disabling.

But, re: 2: What I'm seeing on CD Demo on mobile is an empty "more in this section" block, not the top links. image

(And when I view "generated/post-javascript" source, I don't see top links being hidden by CSS -- but I might not be viewing it correctly, lmk if you see the top links in there.)

ama39 commented 4 years ago

Debugging isn't affecting things -- here's an example on FCS (which has an older version of cwd_base) where the full-width layout / .empty class is working fine: https://dev-fcs-main.pantheonsite.io/test-page-not-menu

If you're talking about how the sidebar completely disappears and extends main content to full width, FCS seems to have something custom. There is nothing in the CSS Framework or cwd_base that works the way this page seems to work. The official way to remove the sidebar is to remove the "sidebar" class from the body tag. But this seems to be actually add a "no-sidebar" class which is used by the FCS theme's stylesheet to alter the page.

But, re: 2: What I'm seeing on CD Demo on mobile is an empty "more in this section" block, not the top links.

There is a menu in there when the page is rendered by Drupal. You can see it if you disable JavaScript before loading the page, and then inspect the markup. My JavaScript removes this effectively-empty menu on page load (client-side).

alisonjo315 commented 4 years ago

Twig debug disabled -- still not getting .empty class: https://0820-legacy-cd-demo.pantheonsite.io/test-page-without-sidebar

There is a menu in there when the page is rendered by Drupal. You can see it if you disable JavaScript before loading the page, and then inspect the markup. My JavaScript removes this effectively-empty menu on page load (client-side).

Yikes, ok. I'm not sure I'm a fan of .empty anymore... Thank you for explaining.

Yes, making the sidebar completely disappear so there's a full-width layout is the whole point :D Sorry I misunderstood the purpose of .empty and went down this tangent...

So, ok, on the page I mentioned (now with twig debug disabled), the menu markup is getting removed, but .empty is not getting added -- maybe still a bug? (Just not a bug I really care about anymore...)

alisonjo315 commented 3 years ago

@ama39 We're having an issue with this on CBB, so, yayyy a reason to work on/fix it!!

Not trying to have an empty sidebar / full-width layout in this case -- in this case, just don't want to have the primary sidebar border happen when there's no primary sidebar: https://dev-cbb.pantheonsite.io/spotlights/winter-2021-test-newsletter

(trying to have full-width in this other case -- as mentioned in Jira BRIGHTBEAMS-38)

ama39 commented 3 years ago

As a reminder, here is a summary of factors involved:

  1. Pages like this do have a block menu rendered by Drupal in the sidebar. So it is not naturally "empty."
  2. Due to limitations with Drupal 8's menu block, the sidebar menu usually shows top-level links we don't want. I hide these with CSS, except for the active path.
  3. I also have some JavaScript that evaluates the sidebar menu, and hides it completely if it is effectively "empty" for our purposes. It's not actually empty, because it will contain the top-level link for the active path.
  4. However, there's also "mobile expander" code that looks for a menu, and processes it for the mobile UI.
  5. There is also JavaScript that looks at the sidebar region as a whole (#sidebar-top) and if it thinks that it is empty, aside from whitespace, it adds a class to help CSS know when it is empty, even though it doesn't qualify as empty according to CSS' logic because of the whitespace. Note: this is the part that may or may not be affected by twig debugging being on.

So what you're looking for here, is for item 5 to be made more robust, to consider more situations to be an "empty" state even though they are not technically so. This should be possible, but it's a larger issue than just the menu. Item 5 is operating at the region-level, so it would have to do more than evaluate the menu. It would have to look for any other blocks or markup in the region. If it's not rigorous enough to account for the unexpected own the road, it could malfunction and hide things erroneously. That's why it hasn't tried to be that sophisticated previously.

alisonjo315 commented 3 years ago

UPDATE: Tone made a change to the JavaScript to improve number 5 (commit #d11837a > cwd_utilities.js)

Meanwhile, Alison has a fix for the menu_block issue on CD Demo, so we won't need the JS/CSS workarounds going forward.