Navigation Screen: Indicate when a menu is deleted and show the menu switcher afterwards #29201
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
grzim
requested review from
adamziel,
kevin940726,
noisysocks and
tellthemachines
as code owners
|
Size Change: +2.5 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
talldan
reviewed
Feb 23, 2021
packages/edit-navigation/src/components/inspector-additions/delete-menu-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menu-selector/style.scss
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/layout/use-navigation-editor.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menu-selector/style.scss
Outdated
Show resolved
Hide resolved
grzim
added
[Package] Edit Navigation
[Type] Enhancement
7 tasks
|
Tested this and works pretty good. |
|
Nice work here. The end to end tests caught a small bug. When creating a first new menu the menu switcher is displayed. The menu just created should be shown instead. It looks like the |
grzim
requested review from
adamsilverstein,
ajitbohra,
ajlende,
chrisvanpatten,
ellatrix,
gziolo,
mkaz,
mkevins,
nerrad,
noahtallen,
oandregal,
ntwb,
spacedmonkey and
TimothyBJacobs
as code owners
grzim
force-pushed
the
add/menu-is-deleted-indicator
branch
from
b776aaa to
bc42639
Compare
grzim
force-pushed
the
add/menu-is-deleted-indicator
branch
from
bc42639 to
d70b07d
Compare
draganescu
force-pushed
the
add/menu-is-deleted-indicator
branch
from
d70b07d to
3b883fe
Compare
|
This seems to work well, but the new "Select menu" screen seems to conflict with #22623 which says that the default behavior in the navigation editor is to edit the last opened menu. Also I should be able to create a new menu right from that screen if we create a new one. Is this select menu screen needed for this PR? |
|
The last step of menu deletion is to show a menu selection window so a user can choose another menu to edit ( the last part of #25435 (comment) ). It is a good catch though, that it should be possible to easily add a new menu, so I suggest adding an |
@draganescu That's how the current editor works, but it's not a cast iron rule. It's really up to a designer like @shaunandrews to define how this works. I think showing the menu switcher initially is a fine option, and if we decide to go with that we can close #22623 once this is merged. The existing end to end tests will need to be updated to cover this change in behaviour.
@grzim This was discussed already in a resolved comment - #29201 (comment) We were both basically working on the same feature at the same time, which wasn't really optimal. Now that #29202 is merged the |
Let's do that. |
Sorry if I contributed to this confusion; I always expected the menu switcher to be the same when accessed from the top area, or when shown after deleting. I literally copy/pasted the elements in Figma for the design.
I don't see these as conflicting statements:
This was already in the design:
|
grzim
force-pushed
the
add/menu-is-deleted-indicator
branch
from
cdf6800 to
b0fa4de
Compare
grzim
force-pushed
the
add/menu-is-deleted-indicator
branch
from
b0fa4de to
3e9db06
Compare
|
I have updated the behavior to work as it used to - when a user navigates to navigator editor, the first menu is selected. This behavior should be changed to selecting the last edited menu, but this is out of the scope of this PR. |
draganescu
reviewed
Mar 25, 2021
talldan
approved these changes
Mar 26, 2021
There was a problem hiding this comment.
This is working well. Thanks for updating it @grzim.
There were a few very small things I noticed in testing, which seem to mostly be because of other changes since this was previously worked on.
Those can be tackled in this PR before merging , or in a separate PR.
edit: I also noticed some of the spacing is slightly wrong when testing:
Design:
In particular there's no spacing above the 'Create new menu' button. Not sure why it looks correct in the dropdown, but not here, but maybe some extra styles can be added to solve it.
packages/edit-navigation/src/components/layout/unselected-menu-state.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/inspector-additions/index.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/inspector-additions/delete-menu-panel.js
Outdated
Show resolved
Hide resolved
|
Thanks, @talldan for testing. When it comes to padding here: which with smaller padding, in my opinion, does not look good: |
|
@grzim There's that padding, but that wasn't the main part I was thinking of:
|
talldan
changed the title
|
Lets merge this and we can follow up with any styling adjustments. |
2 tasks
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
Closes #25435
According to comment the following steps have been added:
deletebutton has abusystatus.How has this been tested?
Tested manually.
I order to test:
delete menubutton in the navbarOKbuttonTypes of changes
New feature
Checklist: