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

[Mobile Nav] Added sliding effect and allowed multiple open accordions #12260

Merged
merged 9 commits into from
May 2, 2024

Conversation

mmmavis
Copy link
Collaborator

@mmmavis mmmavis commented Apr 26, 2024

Description

  • Fixed a few spacing issues.
  • Mobile Nav:
    • added sliding effect
    • allow multiple open accordions

Link to review app: https://foundation-s-feature-ma-36qyhj.herokuapp.com/en/ (CMS cred: admin2/admin2password)
Related PRs/issues: #12233

┆Issue is synchronized with this Jira Story

@mmmavis mmmavis changed the base branch from main to feature-branch/main-nav-dropdown April 26, 2024 21:25
@mmmavis mmmavis changed the title [WIP] Feature/main nav/animation [WIP] Add animation to new nav Apr 26, 2024
@jhonatan-lopes jhonatan-lopes force-pushed the feature-branch/main-nav-dropdown branch from ec8775d to 9128e57 Compare April 29, 2024 19:41
Base automatically changed from feature-branch/main-nav-dropdown to main April 29, 2024 20:21
@mmmavis mmmavis force-pushed the feature/main-nav/animation branch from a907754 to 409d914 Compare April 30, 2024 17:42
@mmmavis mmmavis force-pushed the feature/main-nav/animation branch from 402621d to 45762ae Compare April 30, 2024 18:17
@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj April 30, 2024 18:38 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj April 30, 2024 18:49 Inactive
@mmmavis mmmavis requested review from sabrinang and nancyt1 April 30, 2024 19:59
Copy link

@sabrinang sabrinang left a comment

Choose a reason for hiding this comment

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

Desktop

  • When moving from one item to another, I think there is a quick fade/ease out that makes it look like there's a bit of background flashing through going on (this could be simplified)

Tablet

  • Line below navigation disappears after opening item like “who we are” or after when other items snap to top
  • Border reappears when closing but items do not reset so top item (so “who we are” gets hidden) appears fully hidden
  • The snap to top is working inconsistently for me, but mainly “Blog” moves up a bit more all the time while the others often stay in place which makes it feel inconsistent/unpredictable

Mobile

  • When closing a section other than the first section, a strip is seen of “who we are” but does not fully reset back to the top

I’m thinking overall, having close/open animation without snap to top would provide a more predictable/consistent experience

@nancyt1
Copy link
Collaborator

nancyt1 commented Apr 30, 2024

Mobile

  • Would we be able to animate the initial open and close of the menu?
  • I also noticed that when a nav item is open but i haven't clicked on anything and I close the whole menu, after opening it again the original nav item is still opened. Should we reset once someone closes the menu?
  • +1 to sabrinas comment about not doing snap to top to be more predictable

@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj April 30, 2024 22:47 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj May 1, 2024 00:03 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj May 1, 2024 00:04 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj May 1, 2024 00:17 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj May 1, 2024 16:33 Inactive
@sabrinang
Copy link

sabrinang commented May 1, 2024

The tablet/mobile issues resolved with removing the snap to top combo. Wanted to check in on desktop, if it was possible to not have the background flashing through when transitioning from one primary nav item to another?

Video comparing PR to figma mockup to demonstrate what I mean:
https://github.com/MozillaFoundation/foundation.mozilla.org/assets/2492510/eb87a877-8ccc-4350-a1a4-79072060fa2e

@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj May 1, 2024 19:00 Inactive
@mmmavis mmmavis force-pushed the feature/main-nav/animation branch from 557c3af to 432b499 Compare May 1, 2024 19:01
@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj May 1, 2024 19:01 Inactive
@mmmavis mmmavis temporarily deployed to foundation-s-feature-ma-36qyhj May 1, 2024 19:44 Inactive
@mmmavis mmmavis requested a review from sabrinang May 1, 2024 19:53
@mmmavis mmmavis changed the title [WIP] Add animation to new nav Add animation to new nav May 1, 2024
@mmmavis mmmavis changed the title Add animation to new nav [Mobile Nav] Added sliding effect and allowed multiple open accordions May 1, 2024
@mmmavis mmmavis requested a review from robdivincenzo May 1, 2024 19:56
@mmmavis mmmavis marked this pull request as ready for review May 1, 2024 19:57
Copy link
Collaborator

@robdivincenzo robdivincenzo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀. And very clear comments 📝, thanks Mavis!

@mmmavis mmmavis merged commit c603e75 into main May 2, 2024
11 checks passed
@mmmavis mmmavis deleted the feature/main-nav/animation branch May 2, 2024 16:22
@data-sync-user
Copy link
Collaborator

➤ Simon Acosta Torres commented:

PR has already been merged.

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.

5 participants