Skip to content
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.

Sign up for GitHub

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

Global Styles: Try a simple navigation #33064

Closed
wants to merge 1 commit into from
Closed

Global Styles: Try a simple navigation #33064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* WordPress dependencies
*/
import { __experimentalNavigation as Navigation } from '@wordpress/components';

/**
* Internal dependencies
*/
import { GLOBAL_STYLES_VIEWS, VIEW_ROOT } from './navigation/constants';
import GlobalStylesNavigation from './navigation';
import GlobalStylesCurrentView from './navigation/view';

export default function GlobalStylesV2() {
return (
<GlobalStylesNavigation>
<GlobalStylesView />
</GlobalStylesNavigation>
);
}

function GlobalStylesView() {
return (
<Navigation data={ GLOBAL_STYLES_VIEWS } initial={ VIEW_ROOT }>
Copy link
Contributor

@ciampo ciampo Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

I like that idea 👍

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

I think these are remnants of a previous approach that I didn't clean up. Can be removed.

<GlobalStylesCurrentView />
</Navigation>
);
}
15 changes: 15 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/navigation/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export const VIEW_ROOT = 'root';
export const VIEW_COLORS = 'colors';
export const VIEW_COLORS_PALETTE = 'colors-palette';
export const VIEW_COLORS_ELEMENT = 'colors-element';
export const VIEW_TYPOGRAPHY = 'typography';
export const VIEW_TYPOGRAPHY_ELEMENT = 'typography-element';

export const GLOBAL_STYLES_VIEWS = [
{ title: 'Global Styles', slug: VIEW_ROOT },
{ title: 'Colors', slug: VIEW_COLORS },
{ title: 'Colors Palette', slug: VIEW_COLORS_PALETTE },
{ title: 'Colors Element', slug: VIEW_COLORS_ELEMENT },
{ title: 'Typography', slug: VIEW_TYPOGRAPHY },
{ title: 'Typography Element', slug: VIEW_TYPOGRAPHY_ELEMENT },
];
17 changes: 17 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/navigation/context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* WordPress dependencies
*/
import { createContext, useContext } from '@wordpress/element';

/**
* Internal dependencies
*/
import { VIEW_ROOT } from './constants';

export const GlobalStylesNavigationContext = createContext( {
currentView: VIEW_ROOT,
setCurrentView: () => {},
} );

export const useGlobalStylesNavigationContext = () =>
useContext( GlobalStylesNavigationContext );
22 changes: 22 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/navigation/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import { VIEW_ROOT } from './constants';
import { GlobalStylesNavigationContext } from './context';

export default function GlobalStylesNavigation( { children } ) {
const [ currentView, setCurrentView ] = useState( VIEW_ROOT );

return (
<GlobalStylesNavigationContext.Provider
value={ { currentView, setCurrentView } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should this object be memoized?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

´{ 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that was an oversight of mine 😉

>
{ children }
</GlobalStylesNavigationContext.Provider>
);
}
38 changes: 38 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/navigation/view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* Internal dependencies
*/
import {
VIEW_COLORS,
VIEW_COLORS_ELEMENT,
VIEW_COLORS_PALETTE,
VIEW_ROOT,
VIEW_TYPOGRAPHY,
VIEW_TYPOGRAPHY_ELEMENT,
} from './constants';
import { useGlobalStylesNavigationContext } from './context';
import GlobalStylesViewColors from '../views/colors';
import GlobalStylesViewColorsElement from '../views/colors-element';
import GlobalStylesViewColorsPalette from '../views/colors-palette';
import GlobalStylesViewRoot from '../views/root';
import GlobalStylesViewTypography from '../views/typography';
import GlobalStylesViewTypographyElement from '../views/typography-element';

const allViews = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely. Once we're done with this, it will have to be dynamic, otherwise it's just useless.

[ VIEW_ROOT ]: GlobalStylesViewRoot,
[ VIEW_COLORS ]: GlobalStylesViewColors,
[ VIEW_COLORS_ELEMENT ]: GlobalStylesViewColorsElement,
[ VIEW_COLORS_PALETTE ]: GlobalStylesViewColorsPalette,
[ VIEW_TYPOGRAPHY ]: GlobalStylesViewTypography,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[ VIEW_TYPOGRAPHY_ELEMENT ]: GlobalStylesViewTypographyElement,
};

export default function GlobalStylesCurrentView() {
const { currentView } = useGlobalStylesNavigationContext();
const CurrentView = allViews[ currentView ] ?? null;

return (
<div style={ { padding: '20px 0' } }>
<CurrentView />
</div>
);
}
10 changes: 10 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* Internal dependencies
*/
import GlobalStylesV2 from '../';

export default { title: 'Edit Site (Experimental) /Global Styles v2' };

export const _default = () => {
return <GlobalStylesV2 />;
};
23 changes: 23 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/views/colors-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* Internal dependencies
*/
import { useGlobalStylesNavigationContext } from '../navigation/context';

export default function GlobalStylesViewColorsElement() {
const { setCurrentView } = useGlobalStylesNavigationContext();

const navigateToColors = () => setCurrentView( 'colors' );
const navigateToColorsPalette = () => setCurrentView( 'colors-palette' );

return (
<>
<h3>Colors Element view</h3>
<button onClick={ navigateToColors }>
&lt; Back to Colors view
</button>
<button onClick={ navigateToColorsPalette }>
Go to Colors Palette view &gt;
</button>
</>
);
}
19 changes: 19 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/views/colors-palette.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Internal dependencies
*/
import { useGlobalStylesNavigationContext } from '../navigation/context';

export default function GlobalStylesViewColorsPalette() {
const { setCurrentView } = useGlobalStylesNavigationContext();

const navigateToColorsElement = () => setCurrentView( 'colors-element' );

return (
<>
<h3>Colors Palette view</h3>
<button onClick={ navigateToColorsElement }>
&lt; Back to Colors Element view
</button>
</>
);
}
21 changes: 21 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/views/colors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* Internal dependencies
*/
import { useGlobalStylesNavigationContext } from '../navigation/context';

export default function GlobalStylesViewColors() {
const { setCurrentView } = useGlobalStylesNavigationContext();

const navigateToColorsElement = () => setCurrentView( 'colors-element' );
const navigateToRoot = () => setCurrentView( 'root' );

return (
<>
<h3>Colors view</h3>
<button onClick={ navigateToRoot }>&lt; Back to Root view</button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course 👍

<button onClick={ navigateToColorsElement }>
Go to Colors Element view &gt;
</button>
</>
);
}
21 changes: 21 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/views/root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* Internal dependencies
*/
import { useGlobalStylesNavigationContext } from '../navigation/context';

export default function GlobalStylesViewRoot() {
const { setCurrentView } = useGlobalStylesNavigationContext();

const navigateToColors = () => setCurrentView( 'colors' );
const navigateToTypography = () => setCurrentView( 'typography' );

return (
<>
<h3>Root (main) view</h3>
<button onClick={ navigateToColors }>Go to Colors view &gt;</button>
<button onClick={ navigateToTypography }>
Go to Typography view &gt;
</button>
</>
);
}
19 changes: 19 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/views/typography-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Internal dependencies
*/
import { useGlobalStylesNavigationContext } from '../navigation/context';

export default function GlobalStylesViewTypographyElement() {
const { setCurrentView } = useGlobalStylesNavigationContext();

const navigateToTypography = () => setCurrentView( 'typography' );

return (
<>
<h3>Typography Element view</h3>
<button onClick={ navigateToTypography }>
&lt; Back to Typography view
</button>
</>
);
}
22 changes: 22 additions & 0 deletions packages/edit-site/src/components/sidebar/global-styles-v2/views/typography.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* Internal dependencies
*/
import { useGlobalStylesNavigationContext } from '../navigation/context';

export default function GlobalStylesViewTypography() {
const { setCurrentView } = useGlobalStylesNavigationContext();

const navigateToTypographyElement = () =>
setCurrentView( 'typography-element' );
const navigateToRoot = () => setCurrentView( 'root' );

return (
<>
<h3>Typography view</h3>
<button onClick={ navigateToRoot }>&lt; Back to Root view</button>
<button onClick={ navigateToTypographyElement }>
Go to Typography Element view &gt;
</button>
</>
);
}
1 change: 1 addition & 0 deletions storybook/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but why is this change needed in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order for the global styles story to be included, otherwise it's not.

].filter( Boolean );

const customEnvVariables = {};
Expand Down