Global Styles v2: Move prototype from G2 #32923
Conversation
tyxla
added
[Status] In Progress
|
Size Change: +4.6 kB (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I'm in a very early stage of exploring g2, as well as the prototype, so this doesn't work at all at this time. However, it can give us some ideas as to what to start with to unblock it. Some tasks we can start are:
- Extracting some missing g2 components
- Promoting some experimental components
- Prototyping the global styles state, reusing and extending the existing global styles work.
- Discussing and making decisions wrt the navigation/routing.
I've left some comments and thoughts as I'm pausing work on it for today, just in case they can be helpful if anyone would like to chime in.
| } ); | ||
|
|
||
| // App State | ||
| const initialState = { |
There was a problem hiding this comment.
The state needs a bunch of love. We'll likely need to work on a new @wordpress/data store for this one.
There was a problem hiding this comment.
Will we need to just adopt whatever global styles currently uses for state management? I'm assuming there's a lot of intermediary stuff that needs to be done when the state changes that's already being handled in the current implementation?
There was a problem hiding this comment.
Yes, it makes sense to reuse as much as possible from the existing global styles UI as a whole, not only the state but also the existing components. Which is one more thing we'll have to consider as we move forward to this version.
There was a problem hiding this comment.
Yeah we need to see how global styles are read/written and think about the best way to integrate that into the sidebar
There was a problem hiding this comment.
cc @nosolosw and @jorgefilipecosta here for input
There was a problem hiding this comment.
A good starting point would be to look at the useGlobalStylesContext hook, it gives access to the current settings & styles (see in use). They follow the theme.json specification.
There was a problem hiding this comment.
Another good place to check the logic is the implementation of the GlobalStylesProvider /packages/edit-site/src/components/editor/global-styles-provider.js.
Basically, the source of truth for the global styles user data is in wordpress/data in a post type that contains global styles information. (retrieved using useGlobalStylesEntityContent that just returns the content of the post type useEntityProp( 'postType', 'wp_global_styles', 'content' );).
The provider contains a JSON object representation of that same data (memoized each time the data changes). And also contains mergedStyles the result of merging the user theme.json with the theme and core theme.json data provided by the server.
The provider provides setters and getters for user styles and settings that deal with variable retrieving etc. The consumer of the API just reads and writes values if the values can be represented with CSS variables or come from CSS variables the consumer is not aware.
On each change, there are also effects on the provider that update the compiled CSS of the theme.json tree so the user sees style updates in real-time.
packages/edit-site/src/components/sidebar/global-styles-v2/components/color-picker-modal.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar/global-styles-v2/components/color-picker-modal.js
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,6 @@ | |||
| export { ColorsElementScreen } from './colors-element'; | |||
There was a problem hiding this comment.
Nit: from a dev experience standpoint, is it valid to call these "screens"?
There was a problem hiding this comment.
I think Q was taking a lot of hints from React Native development generally (View and Text component for example) and the navigation matches a lot of how RN does things, including the concept of Screens.
There was a problem hiding this comment.
The thing that can create confusion here is that a "screen" usually takes a whole screen, while here it's just a section of the entire UI.
There was a problem hiding this comment.
Yeah, I'm not sure what to call it. A view?
There was a problem hiding this comment.
I like that better, although it's used for some other things (a View component) 😉 I guess it should be fine regardless since it's an isolated term within the global styles. The problem with isolated terms is that as a project matures, they end up across the entire codebase, and isolated terms end up being not so isolated anymore 😉
There was a problem hiding this comment.
💯
How about GlobalStylesNavigationScreenAndOrViewThing 😂 Surely that won't leak into the rest of the project lol!
In all seriousness though, I agree. Not sure what best to call these things but maybe something specific to global styles would suffice.
There was a problem hiding this comment.
😉
On a serious note, I agree - GlobalStylesScreen or GlobalStylesView would suffice for me.
There was a problem hiding this comment.
I personally prefer View over Screen too.
I also think that we should come up with a solution that works for any sidebars (and "self-contained" pieces of UI), not just the Global Styles sidebar specifically.
I believe Q put some of these concepts behind the Navigation namespace, for example NavigationScreen etc
packages/edit-site/src/components/sidebar/global-styles-v2/screens/typography-element.js
Show resolved
Hide resolved
| import { Screen, ScreenHeader } from '../components'; | ||
| import { useAppState } from '../state'; | ||
|
|
||
| const typographyOptions = [ |
There was a problem hiding this comment.
Seems like this belongs with the global styles state and not specifically with this screen?
packages/edit-site/src/components/sidebar/global-styles-v2/screens/typography-element.js
Outdated
Show resolved
Hide resolved
| }; | ||
| } | ||
|
|
||
| const fontWeights = [ 100, 200, 300, 400, 500, 600, 700, 800, 900 ]; |
There was a problem hiding this comment.
We'll need a more central location for those.
There was a problem hiding this comment.
Currently, only appearance control component is aware of the weights gutenberg/packages/block-editor/src/components/font-appearance-control/index.js. I guess we can keep the same logic and just update that component for the new design (it is experimental).
| */ | ||
| import { useAppState } from '../state'; | ||
|
|
||
| export function ColorPickerModal() { |
There was a problem hiding this comment.
Should we call this one ColorPickerDropdown or Menu? We are looking at something like this, design wise:
| label: o, | ||
| } ) ); | ||
|
|
||
| export const Inspector = () => { |
There was a problem hiding this comment.
Should this be using the default sidebar inspector for blocks? Should it be named something else otherwise?
There was a problem hiding this comment.
Yep, I think we should name this otherwise to prevent from term confusion 😉
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { Item, NavigatorLink } from '@wordpress/components'; |
There was a problem hiding this comment.
Let's think about how we'd reconcile this with the existing navigator set of components currently in use. We can do reconciliation as a separate step, but we should be thinking about it.
There was a problem hiding this comment.
We touched this point briefly in a conversation with @griffbrad — if I remember correctly, Q had created a new set of Navigation components because the existing set did not satisfy his requirements. Am I remembering correctly?
There was a problem hiding this comment.
Correct. Here’s the original conversation for context on the differences: #24107 (comment)
There was a problem hiding this comment.
That's very useful. It also sheds some light on why the Router + Navigation system in this prototype don't use react-router directly (and instead provide a very similar, first-party implementation)
| */ | ||
| import { Surface } from '@wordpress/components'; | ||
|
|
||
| export const Screen = styled( Surface, { props: { variant: 'tertiary' } } )` |
There was a problem hiding this comment.
What's the variant: tertiary expressing?
There was a problem hiding this comment.
It appears to be a mistake: I can't get this API to do anything: https://codesandbox.io/s/competent-grothendieck-weds7?file=/src/App.js
Nor is it documented in Emotion's documentation or types.
There was a problem hiding this comment.
I believe it's a prop of Surface
https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/surface/styles.js#L51
There was a problem hiding this comment.
From the README:
tertiary: Used as the app/site wide background. Visible in dark mode only. Use case is rare.
There was a problem hiding this comment.
Yes it's a prop of Surface but the actual usage here doesn't make any sense or do anything it seems.
|
|
||
| /** | ||
| * These are the "routes" that bind navigation paths with components for the | ||
| * indivudal screens. |
There was a problem hiding this comment.
not sure if we'll keep any of this, but there's a typo "indivudal"
|
|
||
| return ( | ||
| <AppProvider> | ||
| <ColorPickerModal /> |
There was a problem hiding this comment.
Curious why this needs to be included here
| /** | ||
| * External dependencies | ||
| */ | ||
| import { startCase } from 'lodash'; |
| return ( | ||
| <ListGroup> | ||
| <ListGroupHeader>{ title }</ListGroupHeader> | ||
| <HStack alignment="left" wrap> |
There was a problem hiding this comment.
This whole component already exists but is less polished, we should reconcile
There was a problem hiding this comment.
What's the best way to test this UI/PR? I tried to replace the GlobalStylesSidebar component to use the v2 one in packages/edit-site/src/components/sidebar/index.js. But I'm getting an error
styled-base.browser.esm.js:29 Uncaught Error: You are trying to create a styled element with an undefined component.
You may have forgotten to import it.
| ]; | ||
|
|
||
| /** | ||
| * GlobalStylesHeaer and Sidebar can largely be ignored. |
There was a problem hiding this comment.
small typo GlobalStylesHeaer -> GlobalStylesHeader.
| return ( | ||
| <AppProvider> | ||
| <ColorPickerModal /> | ||
| <Navigator initialPath={ initialPath }> |
There was a problem hiding this comment.
I agree I think we can maintain the navigation @wordpress/data or even in react state plus context inside GlobalStylesProvider.
|
|
||
| const handleOnClick = ( i ) => () => { | ||
| appState.setColorPickerKey( | ||
| `color.palettes[${ index }].colors[${ i }].color` |
There was a problem hiding this comment.
We already have getStyle setStyle etc abstractions on packages/edit-site/src/components/editor/global-styles-provider.js.
This logic is complex e.g: when the user sets a color if the color being set is a theme color or user color the color should be referenced using a CSS var instead of the value. When the value is for example CSS custom variable the components should retrieve the value for that value and show it.
Our getters and setters try to abstract everything from developers.
| }; | ||
| } | ||
|
|
||
| const fontWeights = [ 100, 200, 300, 400, 500, 600, 700, 800, 900 ]; |
There was a problem hiding this comment.
Currently, only appearance control component is aware of the weights gutenberg/packages/block-editor/src/components/font-appearance-control/index.js. I guess we can keep the same logic and just update that component for the new design (it is experimental).
| } ); | ||
|
|
||
| // App State | ||
| const initialState = { |
There was a problem hiding this comment.
Another good place to check the logic is the implementation of the GlobalStylesProvider /packages/edit-site/src/components/editor/global-styles-provider.js.
Basically, the source of truth for the global styles user data is in wordpress/data in a post type that contains global styles information. (retrieved using useGlobalStylesEntityContent that just returns the content of the post type useEntityProp( 'postType', 'wp_global_styles', 'content' );).
The provider contains a JSON object representation of that same data (memoized each time the data changes). And also contains mergedStyles the result of merging the user theme.json with the theme and core theme.json data provided by the server.
The provider provides setters and getters for user styles and settings that deal with variable retrieving etc. The consumer of the API just reads and writes values if the values can be represented with CSS variables or come from CSS variables the consumer is not aware.
On each change, there are also effects on the provider that update the compiled CSS of the theme.json tree so the user sees style updates in real-time.
|
Thanks for the feedback @jorgefilipecosta!
It's not testable yet. It's currently in an exploratory phase and there are several big parts to be ironed out first - the biggest one seems to be the navigation / routing part. We'll be looking into reusing as much as possible from the existing v1 functionality. |
|
Thank you all for the great feedback! There's a lot to grok here, and I think we need to set some prerequisites separately before we're able to move this one forward. I've decided to take a simplistic approach with the navigation and opened #33064 for that - would appreciate some 👀 . If we agree on this approach, it will unblock a big part of this PR and allow us to greatly simplify the navigation / routing part, allowing space for some of its features to be built incrementally as we work on the v2 prototype. |
tyxla
force-pushed
the
try/global-styles-sidebar-v2
branch
from
1a77b64
to
636af05
Compare
|
I'm tinkering with moving the minimal amount of some necessary components along from g2 now. This still needs more work, I will continue tomorrow. |
| @@ -91,6 +96,7 @@ export { default as __experimentalNavigationBackButton } from './navigation/back | |||
| export { default as __experimentalNavigationGroup } from './navigation/group'; | |||
| export { default as __experimentalNavigationItem } from './navigation/item'; | |||
| export { default as __experimentalNavigationMenu } from './navigation/menu'; | |||
| export * from './navigator'; | |||
There was a problem hiding this comment.
All the ones we use from here should be __experimental.
| ); | ||
| } | ||
|
|
||
| export default memo( NavigatorSwitch ); |
There was a problem hiding this comment.
Is it necessary to memo this component? Its only prop is children.
There was a problem hiding this comment.
Agreed - I don't see why we'd need a memo here at a first glance. Just moved it as it was part of the prototype PR.
| const contextProps = { | ||
| animationDuration, | ||
| }; |
There was a problem hiding this comment.
Should this be wrapped in useMemo?
| // React 15 compat | ||
| function isModifiedEvent( event ) { | ||
| return !! ( | ||
| event.metaKey || | ||
| event.altKey || | ||
| event.ctrlKey || | ||
| event.shiftKey | ||
| ); | ||
| } |
There was a problem hiding this comment.
Do we need to support React 15?
There was a problem hiding this comment.
I don't think so. I removed some support as I was porting from g2, but this remained. I think we can remove it safely.
| class MemoryRouter extends Component { | ||
| constructor() { | ||
| super(); | ||
|
|
||
| this.history = createHistory( this.props ); | ||
| } | ||
|
|
||
| render() { | ||
| return ( | ||
| <Router children={ this.props.children } history={ this.history } /> | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
We could avoid a class component by using a closure and doing something similar to react-nagivation:
const createMemoryRouter = () => {
let history;
return function MemoryRouter(props) {
history = createHistory(props);
return <Router children={this.props.children} history={history} />;
};
}| // Preact uses an empty array as children by | ||
| // default, so use null if that's the case. |
There was a problem hiding this comment.
Do we need to support Preact?
| /** | ||
| * The public API for putting history on context. | ||
| */ | ||
| class Router extends Component { |
There was a problem hiding this comment.
This can also be a function component.
| /** | ||
| * The public API for rendering the first <Route> that matches. | ||
| */ | ||
| class Switch extends Component { |
There was a problem hiding this comment.
Ditto, function component here too.
| function withRouter( Component ) { | ||
| const displayName = `withRouter(${ | ||
| Component.displayName || Component.name | ||
| })`; | ||
| const C = ( props ) => { | ||
| const { wrappedComponentRef, ...remainingProps } = props; | ||
|
|
||
| return ( | ||
| <RouterContext.Consumer> | ||
| { ( context ) => { | ||
| return ( | ||
| <Component | ||
| { ...remainingProps } | ||
| { ...context } | ||
| ref={ wrappedComponentRef } | ||
| /> | ||
| ); | ||
| } } | ||
| </RouterContext.Consumer> | ||
| ); | ||
| }; | ||
|
|
||
| C.displayName = displayName; | ||
| C.WrappedComponent = Component; | ||
|
|
||
| return hoistNonReactStatics( C, Component ); | ||
| } |
There was a problem hiding this comment.
This should use createHigherOrderComponent from compose.
| import { withRouter } from './router'; | ||
|
|
||
| const withNavigator = withRouter; | ||
|
|
||
| export default withNavigator; |
There was a problem hiding this comment.
Why is this re-export necessary?
There was a problem hiding this comment.
It's structural / organizational IMHO. I don't see a good reason to have it.
| import RouterContext from './router-context'; | ||
|
|
||
| // A public higher-order component to access the imperative API | ||
| function withRouter( Component ) { |
There was a problem hiding this comment.
Do we need this? Why can't we just use useRouter?
There was a problem hiding this comment.
I don't think we'll need it, but it was added likely due to the fact there are a bunch of Component instances lying around.
tyxla
force-pushed
the
try/global-styles-sidebar-v2
branch
from
76db905
to
423dcc9
Compare
| className={ classnames( 'component-color-indicator', className ) } | ||
| className={ classnames( | ||
| 'component-color-indicator', | ||
| { 'is-circular': circular }, |
There was a problem hiding this comment.
This is a workaround, which we may want to go with, or not. The ColorCircle component in g2 seemed a bit more capable, but we should reconsider if we need those capabilities here.
There was a problem hiding this comment.
Conversation started in #33820
| export { default as NavigatorRouter } from './navigator-router'; | ||
| export { default as NavigatorScreen } from './navigator-screen'; | ||
| export { default as NavigatorScreens } from './navigator-screens'; | ||
| export { default as NavigatorSwitch } from './navigator-switch'; |
There was a problem hiding this comment.
We should reconsider if we could ditch any of those navigator components eventually.
| import { useHistory } from './router'; | ||
|
|
||
| function NavigatorButton( props, forwardedRef ) { | ||
| /* eslint-disable no-unused-vars */ |
There was a problem hiding this comment.
Let's fix this instead of disabling the lint error.
| */ | ||
| import { useHistory, useLocation, useParams, useRouteMatch } from './router'; | ||
|
|
||
| export const useNavigatorHistory = useHistory; |
There was a problem hiding this comment.
These re-exports seem to go through several layers, we may want to re-organize and re-export at a single location.
| const combinedProps = { ...routeProps, ...otherProps }; | ||
|
|
||
| /* eslint-disable no-nested-ternary */ | ||
| const content = children |
There was a problem hiding this comment.
This is a ternary nightmare and needs to be fixed to be more comprehensible.
| labelPosition="side" | ||
| /> | ||
| </CardBody> | ||
| <Panel> |
There was a problem hiding this comment.
For the PoC we're using Panel instead of the ListGroup component group from g2. Let's re-evaluate this decision, do we need the ListGroup components here? Or is there anything more suitable than Panel we can use?
| const GlobalStylesHeader = () => { | ||
| return ( | ||
| <CardHeader> | ||
| <Heading level={ 5 }>Global Styles</Heading> |
There was a problem hiding this comment.
Noting that we're intentionally not localizing any strings here, this is just a PoC PR.
| </ZStack> | ||
| </Spacer> | ||
| <View> | ||
| <Text variant="muted">23 colors</Text> |
| <NavLink to="/colors/palette"> | ||
| <HStack> | ||
| <Spacer> | ||
| <ZStack isLayered={ false } offset={ -15 }> |
There was a problem hiding this comment.
We changed this offset to reflect the additional margin that ColorIndicator already contains.
| </RenderComponent> | ||
| <RenderComponent prop={ getIsDefined( 'fontSize' ) }> | ||
| <Grid> | ||
| <FontSizePicker |
There was a problem hiding this comment.
FontSizePicker needs some love in this context. We need to compare it to the g2 implementation and find a better way to have the slider inline with the number field for font size picker.
|
I've pulled the Navigator components from g2, and with some changes, I've made the prototype work in a Storybook story. Here's a very quick demo: gsv2quickdemo.movTo spin it up locally, run There are a lot of visual glitches as you can see, as well as things that are not looking the same way as in the g2 prototype, but those are areas where we'll need to make decisions as to whether to migrate components or adopt the different behavior. As non-mergeable as this PR is, I believe this prototype unveils a whole ton of work for us, including component migration work and unification work. We also still need to figure out how to reuse and adapt as much as possible from the v1 in terms of components, app state and logic. |
ciampo
force-pushed
the
try/global-styles-sidebar-v2
branch
from
bb2b29f
to
09516c3
Compare
|
Rebased and cleaned up some code after merging #33701 (which promoted |
ciampo
force-pushed
the
try/global-styles-sidebar-v2
branch
from
09516c3
to
ed97074
Compare
|
Hi @ciampo, what is the plan to include this sidebar on the edit site? What are the next steps and how can I best help this PR? |
This PR served as a way to start the migration of the prototype into this repo — and consequently to understand better the next steps necessary in order to fully complete this integration. Registering a new version of the Global Styles Sidebar and iterating on it is exactly what we had in mind!
Yes, as we start work on it, we should keep the router/navigation system that comes with the prototype. We can (and should) look into iterating on it as work proceeds! In particular, from my point of view, I can already see how we may need to:
Yes. We would keep iterating on the experimental version, updating all of the necessary dependencies, until the new version is ready to replace the old one. |
|
While we iterate on the other components, we can keep the navigation components local/private to this experimental sidebar implementation for now. We expect they'll ultimately be used in many other contexts, so we'd like to ensure we have a good, general API for them and a good backward compatibility approach relative to the existing Navigation components. However, that doesn't need to hold back iteration on other aspects of the Global Styles sidebar right now. |
|
@griffbrad just wanted to mention that there's no need of a heavy backward compatible approach here, it's fine to replace the existing component if we also replace its usage (only the site editor left sidebar) since it's all experimental. |
Just noting that |
|
Given the different approach that we took for the new version of Global Styles, this is unused and can be closed. |
Description
This is an initial version of the v2 interface of global styles, as moved from ItsJonQ/g2#293 into this repository. Bunch of kudos to @ItsJonQ for the hard work on the prior art.
It is still in progress and needs a bunch of love - addressing navigation/routing, moving some components, promoting some components, and fixing the related imports.
It's not intended to be merged - rather, to inform next discussions and upcoming work.
How has this been tested?
npm run storybook:devTypes of changes
A new feature, foundations for the v2 of Global Styles.
Checklist:
*.native.jsfiles for terms that need renaming or removal).