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

Changes drawer navigation from drawer to accordion #477

Merged
merged 4 commits into from
Aug 4, 2020

Conversation

eduludi
Copy link
Contributor

@eduludi eduludi commented Jul 15, 2020

This replaces the "sub-drawer" behaviour from drawer menu on mobile, with an accordion-like menu.

  • Updates some .c-drawer styles:
    • Subnav arrows points up and down in mobile
    • Increases padding in first and last child items
    • Disables user text selection of menu items (may happen on desktop when user makes a double-click on the arrow)
    • Removes some no longer used styles
    • Updates item and link colors
  • Updates $('.c-primary-nav__arrow').on(click) event
  • Removes placeholder c-drawer__subnav from drawer-navigation.twig (only used to hold the cloned menu item)

- Updates some `.c-drawer` styles
- Updates `$('.c-primary-nav__arrow').on(click)` event
- Removes placeholder `c-drawer__subnav` from  `drawer-navigation.twig`
@@ -14,7 +14,6 @@
<div class="c-drawer__nav">
<div class="c-drawer__nav-primary">
{% include '@molecules/navigation/primary-navigation.twig' %}
<ul class="c-drawer__subnav u-theme--background-color--darker"></ul>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eduludi Is it possible to add a class or CSS change that would make this not necessary to remove? Since we have to coordinate with the update of the Wordpress theme, it would be helpful to be able to leave this line in for at least the next month or so to provide some overlap without messing up the existing installations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't necessary to remove but, as the JS part to make it useful is not longer there, I just did it.

Now I rolled back that change and only two files (CSS and JS) are being modified by this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem I'm seeing is that if you don't remove it, the menu is still unusable on the phone, as it slides out a blank drawer... like before, but with nothing in it. Not sure if that can be hidden?

@designerbrent
Copy link
Collaborator

designerbrent commented Jul 22, 2020

This is still in need of some work, as the video shows, the overlays are still visible with the most recent code.

🎥 Test on mobile

@designerbrent
Copy link
Collaborator

When the menu opens, the indicator needs to be flipped over.
2020-07-22 at 5 07 PM

- It was being triggered in small resolutions after resizing from a larger one
- This caused expand arrows to point in the wrong direction
@eduludi
Copy link
Contributor Author

eduludi commented Jul 23, 2020

This is still in need of some work, as the video shows, the overlays are still visible with the most recent code.

🎥 Test on mobile

This was caused by some CSS selector related to .c-drawer__subnav, so it will be required to remove the element from template (as was my original PR).

We can remove related CSS in a future version, as it still may be used by some implementations consuming the latest CSS.

When the menu opens, the indicator needs to be flipped over.
2020-07-22 at 5 07 PM

I was only able to replicate this when resized the browser from a desktop/ tablet to a mobile resolution. And this was caused because existing script was testing for window's width only at the beginning.

@designerbrent designerbrent merged commit 1036bd2 into adventistchurch:v3.x Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants