Problem/Motivation

Add open close animations to both icons and the submenus.

Doing the submenus is tricky, because you can't animate the max-height between 0 and auto. There's ways around this, such as

  • Instead of display: none the submenu, use visibility: hidden and position: absolute.
  • Doing so, will enable you to use JS to calculate the height of each submenu using something like offsetHeight.
  • Use JS to add that height as the value of an inline custom property.
  • Animate max-height to the value of that custom property
CommentFileSizeAuthor
#38 animation-on-firefox.mp4393.5 KBakshayadhav
#37 firefox.gif253.62 KBckrina
#32 f53a76476b84320ac1d2ed9f80f4506f.gif1.06 MBfinnsky

Issue fork drupal-3374460

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Issue fork navigation-3374460

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

baluv3 created an issue. See original summary.

baluv3 credited mherchel.

baluv3’s picture

baluv3 commented

Moved this issue from https://github.com/Lullabot/navigation and assigned credit.

baluv3’s picture

baluv3 commented
Issue summary: View changes
claireristow’s picture

claireristow commented
Assigned: Unassigned » claireristow

Working on this now!

claireristow’s picture

claireristow commented

Pausing work on this one to work on higher priority tickets. Will pick it up again later.

claireristow’s picture

claireristow commented
Assigned: claireristow » Unassigned

Unassiging in case someone else wants to pick it up

shaal’s picture

shaal
he/him
Primary language English
Location Boca Raton, FL
commented

There's a really nice technique to animate dropdowns using CSS grid.

Ricardocg made their first commit to this issue’s fork.

Ricardocg’s picture

Ricardocg commented

Just added an animation to the menus dropdown. Please review.

Ricardocg’s picture

Ricardocg commented
Status: Active » Needs review

Ricardocg opened merge request !113

finnsky’s picture

finnsky commented

Added couple of comments! Thanks!

Ricardocg’s picture

Ricardocg commented

@finnsky Hi, I think the '&' is necessary for it to work since we do not want to add those two lines of code to all nav items, only to the ones with a dropdown. :)

finnsky’s picture

finnsky commented

@Ricardocg
Please take a look
https://git.drupalcode.org/issue/navigation-3374460/-/commit/b392846e4eb...

Also we have error in js `aria-expanded` :)

Ricardocg’s picture

Ricardocg commented

Hi, as for the scrollIntoView, i can not see any difference compared with the 1.x branch.

Regarding the js error, the change came from the merge with 1.x, and the code is already there (https://git.drupalcode.org/project/navigation/-/blame/1.x/js/sidebar.js). I can try to figure it out, but I do not know the implications it might have.

finnsky’s picture

finnsky commented

This is what not working at the moment on 1.x. 1440x900 screen:

https://gyazo.com/3d8906bb5c301be0b93a6d077f6813a1

It seems works different in this branch, maybe cause of new wrappers, maybe cause of animation delay. Please review.

Ricardocg’s picture

Ricardocg commented

Hi, I used the tugboat of another issue (#3388367) and the scrollIntoView is also not working. Here is a link for you to check: https://mr102-sdg73tajacsrfsxllki9tduxei4xrknq.tugboatqa.com/en/user/1?c...

Should we create a separate issue?

finnsky’s picture

finnsky commented

Still working for me in that tugboat you linked.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Needs work

Thanks @Ricardocg for working on this!! The transition looks really cool!

But I see the same as @finnsky: in my local with the latest code in 1.x the scroll to the active item works well, but not with this MR.

I've also left a few extra comments on the MR, so moving this to Needs work.

Ricardocg opened merge request !114

Ricardocg closed merge request !113

Ricardocg’s picture

Ricardocg commented

I just made a new merge request from a new branch based on the current 1.x since the previews branch was merging changes that i did not make. :)

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

@Ricardocg if this is ready to review the status needs to change to "Need review" :)

I was going to review it but it looks like it will need to be rebased with all the work that happened, so leaving it as Needs work.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs work » Needs review
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Needs work

Actually, moving to NW because it needs to be rebased with the latest code.

BhumikaVarshney changed the visibility of the branch 3374460-add-menu-animations to active.

ckrina changed the visibility of the branch 1.x to hidden.

ckrina changed the visibility of the branch 3374460-add-menu-animations-flat-css to hidden.

ckrina changed the visibility of the branch 3374460-add-menu-animations to hidden.

finnsky’s picture

finnsky commented

I think we also need to make it work with about

https://css-tricks.com/introduction-reduced-motion-media-query/

Gonna work on it

finnsky’s picture

finnsky commented
Issue tags: +Accessibility
StatusFileSize
new f53a76476b84320ac1d2ed9f80f4506f.gif1.06 MB

reduce motion test

finnsky opened merge request !152

finnsky’s picture

finnsky commented

1. Added new media query
--admin-toolbar-reduced-motion (prefers-reduced-motion); to achieve #31

2. Reverted existing transitions for expand collapse sidebar etc for same reason.

3. Applied css transitions with css grid as recommended in #8 Looks good imo.

4. Also added animation for rotate buttons arrows. Not sure if we need it actually.

Please review!

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

ckrina changed the visibility of the branch 3374460-menu-animations to hidden.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
Status: Needs review » Needs work
StatusFileSize
new firefox.gif253.62 KB

4. Also added animation for rotate buttons arrows. Not sure if we need it actually.

I really like this small details, thank you.

It looks great in Chrome, but unfortunately, this doesn't work with Firefox:

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented
StatusFileSize
new animation-on-firefox.mp4393.5 KB

@ckrina
I tested this change on Firefox in Ubuntu. It worked fine on my end.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Project: Navigation » Drupal core
Version: 1.x-dev » 11.x-dev
Component: Code » navigation.module

finnsky changed the visibility of the branch transitions-structurised-3374460 to hidden.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue tags: +Portland2024

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.