Close the dropdown when creating a new menu in the Nav block #43529
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
getdave
added
[Type] Bug
getdave
requested review from
draganescu,
scruffian,
talldan,
MaggieCabrera and
ajlende
|
Size Change: +5 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I'd love to do this on top of #42987 to avoid useless conflicts :) Is it possible? |
16 tasks
|
@draganescu Would you prefer to handle that or would you like me to jump in? |
|
I am not sure but I think porting over the behavior in Fix navigation select menu focus loss into Improves the UX of menu management in the navigation block |
draganescu
added a commit
that referenced
this pull request
Aug 30, 2022
Fixes slow connection UX by disabling and enabling the selector and the manage menus link depending on data status. Co-authored-by: Dave Smith <444434+getdave@users.noreply.github.com>
|
Yes, let's close this as well for #42987 |
draganescu
added a commit
that referenced
this pull request
Sep 2, 2022
Fixes slow connection UX by disabling and enabling the selector and the manage menus link depending on data status. Co-authored-by: Dave Smith <444434+getdave@users.noreply.github.com>
draganescu
added a commit
that referenced
this pull request
Sep 5, 2022
* adds a list view with the navigation block inner blocks to the block's inspector * adds a first toolpanel item * stub toolpanel item * moves the menu selector block controls to the inspector * fixes label update bug * labels navigation menu selector if no menus or deleted menu, fixes label when importing classic menu * adds the chevron * only show no menus label if no block menus exist, disregard classic menus * adds check for permissions on the manage menus link; removes rogue showManageActions * Implements handling focus persistence in the Navigation Selector according to #42956 Switching between menus keeps the menu open and focused on the current selection. Importing classic menus or creating new ones focuses the current navigation block. Co-authored-by: Daniel Richards <677833+talldan@users.noreply.github.com> * It looks like `hasManagePermissions` is not cached between API calls so we refresh this quite often when switching between menus. For a more consistent UI experience we disabe the menu management link untill `hasManagePermissions` is refreshed. Another option would be to cache this at a component level. But, this could be more accurate? * Closes the dropdown when importing or creating new menu as in #43529 Fixes slow connection UX by disabling and enabling the selector and the manage menus link depending on data status. Co-authored-by: Dave Smith <444434+getdave@users.noreply.github.com> * Larger commit, adds: - The label of the currently selected menu updates if menu is renamed from advanced - Uses a visually hidden label to explain this is a menu switcher - Defaults to a button if there are no block menus and no classic menus - Renames "Loading options ..." to "Loading ..." Co-authored-by: Javier Arce <4933+javierarce@users.noreply.github.com> Co-authored-by: Alex Stine <13755480+alexstine@users.noreply.github.com> * move styles to editor.scss as they are only needed for the editor * use flex rather than absolute positioning * alphabetize CSS properties * Incorporates review feedback: - replaces disabled with isBusy for toggle working state - refactores some conditionals for better readability - adds consistency to the toggle label Co-authored-by: Daniel Richards <677833+talldan@users.noreply.github.com> Co-authored-by: Dave Smith <444434+getdave@users.noreply.github.com> Co-authored-by: Javier Arce <4933+javierarce@users.noreply.github.com> Co-authored-by: Paal Joachim Romdahl <5323259+paaljoachim@users.noreply.github.com> Co-authored-by: Daniel Richards <677833+talldan@users.noreply.github.com> Co-authored-by: Dave Smith <444434+getdave@users.noreply.github.com> Co-authored-by: Javier Arce <4933+javierarce@users.noreply.github.com> Co-authored-by: Alex Stine <13755480+alexstine@users.noreply.github.com> Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: Paal Joachim Romdahl <5323259+paaljoachim@users.noreply.github.com>
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.
What?
Fixes the Nav block so that when you click
Create new menuthe UI responds immediately and the dropdown menu is closed.Also protects against repeatedly triggering the "create" or "convert classic" actions by hiding the dropdown meny entirely if one of these actions is in progress.
Question: is it ok to remove the Select Menu from the DOM due to a11y focus concerns?
Why?
Previously if you click on
Create new menuunder theSelect Menudropdown the UI would appear unresponsive in that the dropdown would remain open. Granted thanks to previous work the block showed some feedback as to the progress but the dropdown obsecured this.What's worse is that because the
Create new menuoption wasn't disabled, you could simply click the menu option multiple times leading to multiple copies of the same Navigation being created.You could also do the same with the convert classic menus action.
How?
This PR calls the
onClosecallback to ensure that the dropdown is closed as soon as you click on theCreate new menuoption. This ensures that the previous problems are removed.It also ensures that the dropdown menu is not in the DOM if either creating a menu or converting a classic menu.
Testing Instructions
It's worth trying the following on a slow network connection (tip: use dev tools!) to really see the difference between this PR and
trunk:Select menuand then clickCreate new menu.Repeat the above but this time try converting a classic menu instead.
Screenshots or screencast
Screen.Capture.on.2022-08-23.at.16-49-52.mov