Global Styles: Try a simple navigation #33064
Conversation
tyxla
added
the
Global Styles
tyxla
requested review from
mtias,
oandregal,
ciampo,
jorgefilipecosta and
sarayourfriend
|
Size Change: 0 B Total Size: 1.05 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
This LGTM.
Just a couple notes from our sychronous discussion earlier:
We will manage a navigation stack of slugs (like view breadcrumbs) each time a View is entered into. Using these slugs we'll be able to retrieve any relevant metadata about the current and previous views. This will allow us to programatically set the view header and back buttons to the correct values as well as render the correct view when hitting the back buttons.
Do we also need to think of a Link type component like react-router has that will facilitate those navigations/pop things into the stack and update the current view? Or is using the setCurrentView function good enough?
|
|
||
| return ( | ||
| <GlobalStylesNavigationContext.Provider | ||
| value={ { currentView, setCurrentView } } |
There was a problem hiding this comment.
Nit: should this object be memoized?
There was a problem hiding this comment.
Thank you for proposing a simple navigation system @tyxla 👍 I really like this approach.
If possible it would be good to see how this system expands for the dynamic view/routes case as for sure we will need to have views per each block and it seems to only thing not covered.
We can simulate for example that inside the root we have a blocks view, and then inside this blocks view for each element of an array we have another view (for each block) and inside the view of each block, we can have all the global views again (excluding blocks for now because we don't have styles nesting yet).
| return ( | ||
| <> | ||
| <h3>Colors view</h3> | ||
| <button onClick={ navigateToRoot }>< Back to Root view</button> |
There was a problem hiding this comment.
This is just for demo purposes for now, but it already shows that we probably need general components with things like the back button to go back to previews view, the title of the view etc.
|
|
||
| return ( | ||
| <GlobalStylesNavigationContext.Provider | ||
| value={ { currentView, setCurrentView } } |
There was a problem hiding this comment.
´{ currentView, setCurrentView }` creates a new object every time the component rerenders, given that this is a context value I guess it will trigger unrequited rerender on the components tree. Maybe in this case useMemo is helpful.
There was a problem hiding this comment.
Yep, that was an oversight of mine 😉
| import GlobalStylesViewTypography from '../views/typography'; | ||
| import GlobalStylesViewTypographyElement from '../views/typography-element'; | ||
|
|
||
| const allViews = { |
There was a problem hiding this comment.
This may grow huge with all the subviews. Maybe the root view does not need to be aware of the subviews and just of its children, and then its children are aware of its subviews.
Another alternative is importing the subviews from the children.
import { views as colorViews } from '../views/colors'
const allViews = {
...colorViews,
}
But for now keeping them in a central place is ok we don't know yet if it is going to be a problem or not.
There was a problem hiding this comment.
Absolutely. Once we're done with this, it will have to be dynamic, otherwise it's just useless.
| [ VIEW_COLORS ]: GlobalStylesViewColors, | ||
| [ VIEW_COLORS_ELEMENT ]: GlobalStylesViewColorsElement, | ||
| [ VIEW_COLORS_PALETTE ]: GlobalStylesViewColorsPalette, | ||
| [ VIEW_TYPOGRAPHY ]: GlobalStylesViewTypography, |
There was a problem hiding this comment.
I may want to change the typography settings of block A I just installed. So in practice all these views could also be nested e.g: I may go to root-> view typography to change typography settings for all the site, or I may go to root -> blocks -> block a (dynamic depending on the blocks of site) -> view typography.
It would be nice to see how this system expands to this dynamic case. E.g: we may have a const all blocks = [] that simulates the blocks on the website. And then we could simulate this "dynamic" view case.
It is not a blocker we can git it a try in a follow-up PR.
There was a problem hiding this comment.
Good call 👍 Once we have this running with the prototype, we can start experimenting with the nested and dynamic views, and it's a good idea to do it sooner than later.
|
Lots of great feedback, thank you, folks! I think we have to take most (if not all) of your suggestions for the long-term navigation solution. My point was to have an initial version of this so we can get the prototype running and to a place where we can actually see it, just like we can in the g2 repo. My next steps are to merge this PR into #32923 and make them work together to allow spinning the new interface in a storybook. From there we can advance with component work, navigation, data, and all the other moving pieces in separate PRs. |
|
As a potential alternative to implementing a new simpler navigation, have we considered just keeping the routing + navigation system from the existing prototype? Since the medium/long term plan is to come up with a more complex and comprehensive routing solution, for now we could just keep what is already in the prototype since it seems to be working well enough and has reasonable APIs — and then swap the implementation with a more robust / well-thought-out at a later point |
I personally haven't, but I agree with giving it a try in any case. There's a lot we can learn from playing with moving it along from g2. |
Got it! The reason why I was suggesting to keep whatever routing solution is currently implemented in the prototype is that I wasn't aware that it is a blocker preventing the rest of the migration from happening
What aspects of the navigation/router logic are blocking the migration? |
| @@ -8,6 +8,7 @@ const stories = [ | |||
| '../packages/block-editor/src/**/stories/*.js', | |||
| '../packages/components/src/**/stories/*.js', | |||
| '../packages/icons/src/**/stories/*.js', | |||
| '../packages/edit-site/src/**/stories/*.js', | |||
There was a problem hiding this comment.
Maybe I'm missing something, but why is this change needed in this PR?
There was a problem hiding this comment.
In order for the global styles story to be included, otherwise it's not.
|
|
||
| function GlobalStylesView() { | ||
| return ( | ||
| <Navigation data={ GLOBAL_STYLES_VIEWS } initial={ VIEW_ROOT }> |
There was a problem hiding this comment.
It seems like we're "defaulting" to VIEW_ROOT as the initial view in 2 places (here and when setting the initial value of currentView in packages/edit-site/src/components/sidebar/global-styles-v2/navigation/index.js), should we aim at having it defined only in one place as the initial/starting view?
There was a problem hiding this comment.
Also, I'm not very familiar with this component, but after a quick look I can't see the data and initial props being defined in packages/components/src/navigation/index.js
There was a problem hiding this comment.
It seems like we're "defaulting" to
VIEW_ROOTas the initial view in 2 places (here and when setting the initial value ofcurrentViewinpackages/edit-site/src/components/sidebar/global-styles-v2/navigation/index.js), should we aim at having it defined only in one place as the initial/starting view?
I like that idea 👍
Also, I'm not very familiar with this component, but after a quick look I can't see the
dataandinitialprops being defined inpackages/components/src/navigation/index.js
I think these are remnants of a previous approach that I didn't clean up. Can be removed.
Just the fact that the components and functionality used for navigation and routing in g2 are not present in Gutenberg right now, and I'm not convinced we want to just move them here. |
|
Thank you all for the feedback! As we discussed yesterday with @diegohaz, @sarayourfriend, @ciampo, and @griffbrad, I'd like to try out moving the g2 navigation components here early next week. Considering that their API is richer and close to |
|
Now that we've used the navigation and routing from g2 in #32923, I think we can close this one. |
Description
One of the bigger blockers for moving #32923 forward (moving global styles v2 from g2 to Gutenberg) is the navigation/router logic. There's a lot to discuss there when it comes to how functional and "routery" we want it to be, whether we need advanced routing (with react router or so), URL persistence, history management, a dedicated data store, animations, transitions, etc. However, to me, none of that is a blocker for the initial implementation and all of those could be incrementally added to the project.
So what we really need to move the project forward is a simple navigation system that's responsible for holding all the views (I'm adopting that term as per this discussion), maintaining and presenting the current view and allowing navigation between all the views. That's all we need in order to make progress for now, and all the rest could be considered a feature and/or eye candy. If we land this, it will help unblock much of #32923 and allow us to actually get much closer to a working prototype.
So this PR sets the stage with a very simple navigation system that's based on the existing
Navigationcomponent base and uses the Context API. We're introducing the views that are introduced in #32923 as simple empty placeholder components with some navigational items inside them, which we're currently using just for demonstration. Since that's not imported anywhere, we can use that stage and further build the Global Styles v2 prototype on top of it.How has this been tested?
This is manually testable through a new Storybook story:
npm run storybook:devTypes of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).