-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Try/sub nav editing appender #22165
Try/sub nav editing appender #22165
Conversation
Size Change: +154 B (0%) Total Size: 822 kB
ℹ️ View Unchanged
|
Yes this is great! As my comment explained it's a bit weird to be suddenly without the appender and be forced to use the toolbar. This way it is at least predictable when the appender appears. |
I like it. Just going to loop in @jasmussen to be sure we're keeping to patterns that are expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a design perspective going to approve, thanks.
Yep, seems like a good small win! |
This felt natural at first, but then I noticed some situations where there's no appender, typically when there are navigation links selected and they have no children: Nested block with no childrenRoot level block with no childrenI think this may end up being confusing to some users who just want to add another sibling block, but I'm not sure what the best solution would be. Maybe the appender at the same level as the block should be visible if the selected block has no children? |
I'm having some issues with my environment so I can't test this — but is it related to what has focus, @talldan? Any menu item can have both item focus and text editing focus. |
@jasmussen It seems to happen with either kind of focus. |
@talldan this should be fixed now, would you confirm please? Also CC @karmatosed |
Looking at the gif, that's awesome! Thanks for working on this as I really believe only having one appender makes a lot more sense. |
Finally this feels natural :D |
@adamziel: Did you mean to merge this? I'm not seeing these changes in |
@noisysocks ah good spot, thank you! I just spinned a new PR here: #22311 |
Description
This is a follow-up on #22107 to remove the appender from non-active navigation blocks:
I extracted this into a separate PR because it broke multiple tests and I didn't want it to block the other changes.
How has this been tested?
Types of changes
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Checklist: