Refactor the colors UI in the inspector to use the navigator approach#36952
Closed
youknowriad wants to merge 2 commits intotrunkfrom
Closed
Refactor the colors UI in the inspector to use the navigator approach#36952youknowriad wants to merge 2 commits intotrunkfrom
youknowriad wants to merge 2 commits intotrunkfrom
Conversation
|
Size Change: +735 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
mtias
reviewed
Nov 29, 2021
|
|
||
| { hasTextColor && ( | ||
| <NavigatorScreen path="/text"> | ||
| <ScreenHeader back="/" title={ __( 'Text' ) } /> |
Member
There was a problem hiding this comment.
These "back" links need the ability to focus the element they represent when you go back, otherwise the top one is always selected. This also happens in global styles.
Contributor
Author
There was a problem hiding this comment.
mmm, this might not be easy without introducing some kind of #hash support in the "path" since the original element is remounted entirely.
Member
Contributor
There was a problem hiding this comment.
What if the NavigatorScreen had the ability to move the focus on itself (or on a children) when mounted?
Contributor
Author
There was a problem hiding this comment.
For me the only to know the element that was focused before moving to a new "screen" is:
- Keep a reference to the old element
- Or have an identifier for the element.
We don't render all the screens at the same time, meaning, even if keep a reference, when going back to the previous screen, new elements will be rendered so that reference that was kept is useless.
Meaning, we only have one option: add identifiers to elements and pass the identifier somehow to navigator.push calls.
Contributor
Author
There was a problem hiding this comment.
we can explore keeping hidden screens mounted
That would be a huge performance degradation for Global Styles, there are hundreds if not thousands of screens (blocks...)
Contributor
There was a problem hiding this comment.
Would moving the focus on the new "screen" (instead that on the precise link) be an acceptable compromise? That would simplify the solution considerably (we'd just need to add the option for a screen to auto-focus when mounted)
Member
There was a problem hiding this comment.
Let's move this conversation into an issue specifically for global styles, since we might not end up doing anything here on the block inspector.
Contributor
|
This PR is incredible, and validates the idea of consolidating colors in itemgroups: There are a few small things here and there, which I'd love to help with. Let me know if/when and how I might help. |
7 tasks
Contributor
|
This PR will benefit from a few things happening separately:
Here's what's left:
The focus handling is already being discussed. So what remains after that is managing the hierarchy and treatment of the "Background" subheading, the border below, and the margins/paddings that make this panel look a bit compressed. I'll dive in and see if there's any small and clean tweaks I can push there. |
Contributor
|
I pushed a small change to absorb the double padding: It's hard to do this without a little bespoke CSS (see 29c49a8), because:
I took the liberty of pushing the code, but feel free to comment on whether there's a better way to accomplish this. One thing that I had to unset was an Depending on whether we can accept these changes, my next fix would be to polish the back button and the subheading. |
Contributor
|
Seems very complicated (and maybe a dangerous precedence) to mix the accordion with the drill down. This could lead to having multiple accordions each with their own drill down, which seems to add a lot of cognitive overhead. |
Contributor
Author
|
Going to close this one for now, I think we should land #37053 instead for 5.9. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
This begins to implement the ability to group color elements in the block inspector based on the design flow of #34574.
This is just a naive approach to see what it would give as a result.