HDS-1975 mobile submenu fixes #1182
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NikoHelle
added
the
waiting-for-accessibility-audit
NikoHelle
force-pushed
the
HDS-1975-submenu-fix
branch
3 times, most recently
from
a8ba12c to
049bd64
Compare
|
Check if this PR fixes issue in doc site header examples. If not, a separate ticket should be made. What happens in the video is that the closed mobile menu is shown over the Header. It can't be hidden because then transition animation wouldn't work. Screen.Recording.2023-12-28.at.10.06.17.mov |
NikoHelle
force-pushed
the
HDS-1975-submenu-fix
branch
from
049bd64 to
fa46ee6
Compare
NikoHelle
removed
the
waiting-for-accessibility-audit
Same code was repeated too many times.
Current state based system stores a lot of information in different React states. The menus do not change, just the selected ones. So storing selected menu indexes makes the data structure simpler. State based rendering has problem with data updates. Some rendered data is old until state changes. That's why there is the bug with rendering correct menus when animating.
Menu position can be set immediately. Render count is reduced.
The id was used just for focusing. The id was not very unique, so might conflict.
The menu never had the id that the menu button 's aria-controls refers to.
Moved the drop to the dropdown button
Another PR made the mobileMenu element always visible and animates it vertically.
NikoHelle
force-pushed
the
HDS-1975-submenu-fix
branch
from
fa46ee6 to
0ca84bc
Compare
|
Issue in Harri's video still occurs after this. Ticket was created: https://helsinkisolutionoffice.atlassian.net/browse/HDS-2095 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Mobile menu had a few issues:
The
useStatebased rendering system was replaced with an index based system, where each selected menu is stored only as an array of data. Previously whole menu contents were stored and therefore updates were not in sync and updated too late.With index based data the rendering does not depend on data in state, and is always up to date. And the code is simpler.
Note: vertical animation came after this PR was initially done and made the animation handling more complicated. This PR is now fixed to handle both animated transforms.
Related Issue
Closes HDS-1975
How Has This Been Tested?
Added missing unit tests. Tested locally.
Demo
See related PR