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

WD-2617 - Add sliding animation to sub navigation on mobile #4714

Conversation

pedoch
Copy link
Contributor

@pedoch pedoch commented Mar 23, 2023

Done

  • Add styles for the sliding navigation
  • Add new example for the sliding navigation

Fixes/Issues

QA

  • Open demo
  • Check out the Navigation / Sliding under components and make it behaves as expected.
  • Check out the other navigation examples as well and make sure they're still working like they used to.
  • Review updated documentation:
    • [List any updated documentation for review]

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix relesase (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

[if relevant, include a screenshot or screen capture]

@webteam-app
Copy link

Demo starting at https://vanilla-framework-4714.demos.haus

bartaz

This comment was marked as outdated.

@bartaz

This comment was marked as outdated.

@pedoch

This comment was marked as resolved.

@bartaz

This comment was marked as resolved.

@ClementChaumel
Copy link
Contributor

ClementChaumel commented Mar 30, 2023

@bartaz @pedoch oops I updated this branch directly instead of creating a PR targeting it. Let me know if it's ok.
If we're happy with the js export here I think it's fine but if there's more discussion to be had, I'll revert it and open it in its own PR so as to not pollute this one.

here I add the slidingNav function directly to the window object. I think moving forward we'd want to maybe bundle everything under a vanillaFramework property to be called like

vanillaFramework.slidingNav('element')

As a side, I tried to use rollup to mimic what we do in the cookie policy repo but I couldn't get it working so I opted for esbuild instead.

@bartaz bartaz force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from 8c28a72 to 6bb0a60 Compare March 31, 2023 10:50
@bartaz
Copy link
Contributor

bartaz commented Mar 31, 2023

@bartaz @pedoch oops I updated this branch directly instead of creating a PR targeting it. Let me know if it's ok. If we're happy with the js export here I think it's fine but if there's more discussion to be had, I'll revert it and open it in its own PR so as to not pollute this one.

@ClementChaumel I just reverted your commit, it was causing some CI checks fail, and demo didn't run, so make a separate PR and make sure it runs (I guess docker config may need updating to include the built JS file).

here I add the slidingNav function directly to the window object. I think moving forward we'd want to maybe bundle everything under a vanillaFramework property to be called like

vanillaFramework.slidingNav('element')

As a side, I tried to use rollup to mimic what we do in the cookie policy repo but I couldn't get it working so I opted for esbuild instead.

I think we should do it from the start. Once we settle on JS API and expose it we don't want to change it. So good to make it future-proof from the start.

So something like:

vanillaFramework.setupMobileNavigation();
vf.setupMobileNavigation();

We can discuss details in your separate PR I guess.

bartaz

This comment was marked as resolved.

@bartaz

This comment was marked as resolved.

@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch 3 times, most recently from 1e8293e to 36420a9 Compare April 4, 2023 08:41
@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from 36420a9 to da06e22 Compare April 5, 2023 08:31
@bartaz
Copy link
Contributor

bartaz commented Apr 5, 2023

Thanks for all the updates. Two small issues remaining:

When you open a dropdown (in mobile mode) and resize screen, dropdown gets closed (but menu remains open). But some class names must be incorrect, because you have to click TWICE on the menu item to reopen this dropdown again.

panels-hiding

Second issue is when screen is smaller than menu (for example in mobile landscape mode). When you open dropdown with small number of items you can scroll down and reveal the previous panel.

panels-landscape

@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch 2 times, most recently from 747b985 to 38a7d4f Compare April 5, 2023 10:24
@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from 054a4c6 to 28c8af6 Compare April 6, 2023 08:13
- add the styling and the example for the sliding navigation
- fix some lint issues and code review comments
drivebys:
- fix scss linter max-nesting-depth error
- correct html mark up
- implement the animation use the same concept as the side navigation
- use new menu button
- make navigation separtor be on top
- fixed unexpected animation
- implement pressed state background on the navigation
- fix chevron rotation on dropdown toggle
- create new classname for back button (stick with convention)
- prevent parent slide from scrolling when content is longer
than the viewport and child slide is open
- for alpha test
- minor update
@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from 16e1eb7 to 78ce9eb Compare April 11, 2023 10:46
@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from faacd51 to aa9e8e6 Compare April 19, 2023 15:05
@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from aa9e8e6 to 9b41447 Compare April 19, 2023 15:47
@bartaz
Copy link
Contributor

bartaz commented Apr 20, 2023

@pedoch Good work on the animation!

Few small issues I noticed:

  1. It is possible now to open multiple dropdowns on desktop view, this should not happen, opening one should close the others:

nav-multiple-dropdowns

  1. There is some weird issue with disappearing navigation items.

nav-hiding

I think a way to reproduce it is:

  • go to mobile view, open the navigation dropdown
  • make screen bigger into desktop view
  • click on any dropdown
  • all navigation disappears

I guess some class names are not removed and mobile styles are applied?

@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from f962bde to 22e578a Compare April 20, 2023 16:06
- add new search example
- fix navigation bugs
@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from 22e578a to 5f5a194 Compare April 20, 2023 16:57
@juanruitina
Copy link

Discussing on Jira.

@@ -0,0 +1,67 @@
function initNavigationSearch(element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't looked into details, but how different it is from existing script for search? https://github.com/canonical/vanilla-framework/blob/main/templates/docs/examples/patterns/navigation/_script-search.js

This looks to be share a lot, would it be possible for one script to handle both existing version and sliding version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll try combining the file so they work for both versions of the navigation

@pedoch pedoch force-pushed the WD-2617-add-sliding-animation-to-sub-navigation-on-mobile branch from 560e88c to 887c244 Compare April 25, 2023 14:44
The overlay makes the animation resemble that of iOS
@ClementChaumel
Copy link
Contributor

@juanruitina @lyubomir-popov would something like that be ok? (I slowed it down to demonstrate better what's happening but it would be quicker ofc)

slidingnav

@bartaz bartaz mentioned this pull request May 18, 2023
3 tasks
@pedoch pedoch closed this May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants