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

Font Library: Use Interface datastore for modal state management #58350

Closed
wants to merge 1 commit into from
Closed

Font Library: Use Interface datastore for modal state management #58350

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
11 changes: 7 additions & 4 deletions packages/edit-site/src/components/app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { privateApis as routerPrivateApis } from '@wordpress/router';
*/
import Layout from '../layout';
import { GlobalStylesProvider } from '../global-styles/global-styles-provider';
import FontLibraryProvider from '../global-styles/font-library-provider';
import { unlock } from '../../lock-unlock';

const { RouterProvider } = unlock( routerPrivateApis );
Expand All @@ -37,10 +38,12 @@ export default function App() {
<SlotFillProvider>
<GlobalStylesProvider>
<UnsavedChangesWarning />
<RouterProvider>
<Layout />
<PluginArea onError={ onPluginAreaError } />
</RouterProvider>
<FontLibraryProvider>
<RouterProvider>
<Layout />
<PluginArea onError={ onPluginAreaError } />
</RouterProvider>
</FontLibraryProvider>
Comment on lines +41 to +46
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@youknowriad Can you let me know if there are any issues with adding a new provider to this level?

The Font Library shares context in the Typography panel of the Global Style sidebar and in the modal dialog. By moving the modal dialog here, I needed to add a provider at this level to continue sharing context.

Judging by performance tests, there doesn't seem to be any performance degradation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what is this provider about?

Copy link
Contributor

Choose a reason for hiding this comment

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

Took a look at it (very quickly), it seems like a random provider to share some props and things like that. I'm not entirely sure that we conceptually need a context at all for this. Can't we work on removing it entirely?

Copy link
Contributor Author

@t-hamano t-hamano Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can't we work on removing it entirely?

Yes, I think most of the context here can be removed with proper refactoring.

However, I think there is one context that must be shared between the modal dialog and the typography panel. We need to dynamically switch the content of a modal dialog depending on whether I click on an icon in the typography panel or on any font. Is there a way to achieve this behavior without using a provider?

e19a757fd9fba2e533186da827df815e.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are to move the modal open/close state to the interface package, then yes, context is an option. But it feels like we could have some "initialState" argument that we can pass to openModal.

In other words, how do we do this in other modals? Or maybe we didn't have a use-case in other modals to not open a modal in its initial state.

--
Regardless of this discussion, and since we have the context today, I think it's fine to add it top-level with the caveat that we should move away from it. Conceptually edit-site/app/index should be seen as the future wp-admin index. It seems that adding random providers there is not a great way to achieve things. "Routes" need to be lazy loaded basically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe we didn't have a use-case in other modals to not open a modal in its initial state.

As far as I've researched, there doesn't seem to be any such use case. I found #54199, which is similar to this approach, but I don't know if this issue is intended to control the initial state of the modal.

Personally, I don't want to rush this PR as there are other things that could be improved regarding the Font Library.

Should we consider adding new arguments to openModal after removing the context as far as possible and then come back to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider adding new arguments to openModal after removing the context as far as possible and then come back to this PR?

Yes, I'd like that ❤️

</GlobalStylesProvider>
</SlotFillProvider>
);
Expand Down
83 changes: 32 additions & 51 deletions packages/edit-site/src/components/global-styles/font-families.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useDispatch } from '@wordpress/data';
import {
__experimentalItemGroup as ItemGroup,
__experimentalVStack as VStack,
Expand All @@ -11,67 +12,47 @@ import {
} from '@wordpress/components';
import { typography } from '@wordpress/icons';
import { useContext } from '@wordpress/element';
import { store as interfaceStore } from '@wordpress/interface';

/**
* Internal dependencies
*/
import FontLibraryProvider, {
FontLibraryContext,
} from './font-library-modal/context';
import FontLibraryModal from './font-library-modal';
import { FontLibraryContext } from './font-library-provider';
import FontFamilyItem from './font-family-item';
import Subtitle from './subtitle';
import { FONT_LIBRARY_MODAL_NAME } from './font-library-modal';

function FontFamilies() {
const { modalTabOpen, toggleModal, themeFonts, customFonts } =
useContext( FontLibraryContext );

export default function FontFamilies() {
const { themeFonts, customFonts } = useContext( FontLibraryContext );
const { openModal } = useDispatch( interfaceStore );
const hasFonts = 0 < customFonts.length || 0 < themeFonts.length;

return (
<>
{ !! modalTabOpen && (
<FontLibraryModal
onRequestClose={ () => toggleModal() }
initialTabName={ modalTabOpen }
/>
) }

<VStack spacing={ 3 }>
<HStack justify="space-between">
<Subtitle level={ 3 }>{ __( 'Fonts' ) }</Subtitle>
<HStack justify="flex-end">
<Tooltip text={ __( 'Manage fonts' ) }>
<Button
onClick={ () =>
toggleModal( 'installed-fonts' )
}
aria-label={ __( 'Manage fonts' ) }
icon={ typography }
size={ 'small' }
/>
</Tooltip>
</HStack>
<VStack spacing={ 3 }>
<HStack justify="space-between">
<Subtitle level={ 3 }>{ __( 'Fonts' ) }</Subtitle>
<HStack justify="flex-end">
<Tooltip text={ __( 'Manage fonts' ) }>
<Button
onClick={ () =>
openModal( FONT_LIBRARY_MODAL_NAME )
}
aria-label={ __( 'Manage fonts' ) }
icon={ typography }
size={ 'small' }
/>
</Tooltip>
</HStack>
{ hasFonts ? (
<ItemGroup isBordered isSeparated>
{ customFonts.map( ( font ) => (
<FontFamilyItem key={ font.slug } font={ font } />
) ) }
{ themeFonts.map( ( font ) => (
<FontFamilyItem key={ font.slug } font={ font } />
) ) }
</ItemGroup>
) : (
<>{ __( 'No fonts installed.' ) }</>
) }
</VStack>
</>
</HStack>
{ hasFonts ? (
<ItemGroup isBordered isSeparated>
{ [ ...customFonts, ...themeFonts ].map( ( font ) => (
<FontFamilyItem key={ font.slug } font={ font } />
) ) }
</ItemGroup>
) : (
<>{ __( 'No fonts installed.' ) }</>
) }
</VStack>
);
}

export default ( { ...props } ) => (
<FontLibraryProvider>
<FontFamilies { ...props } />
</FontLibraryProvider>
);
11 changes: 7 additions & 4 deletions packages/edit-site/src/components/global-styles/font-family-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,31 @@
* WordPress dependencies
*/
import { _n, sprintf } from '@wordpress/i18n';
import { useDispatch } from '@wordpress/data';
import {
__experimentalHStack as HStack,
__experimentalItem as Item,
FlexItem,
} from '@wordpress/components';
import { useContext } from '@wordpress/element';
import { store as interfaceStore } from '@wordpress/interface';

/**
* Internal dependencies
*/
import { FontLibraryContext } from './font-library-modal/context';
import { FontLibraryContext } from './font-library-provider';
import { getFamilyPreviewStyle } from './font-library-modal/utils/preview-styles';
import { FONT_LIBRARY_MODAL_NAME } from './font-library-modal';

function FontFamilyItem( { font } ) {
const { handleSetLibraryFontSelected, toggleModal } =
useContext( FontLibraryContext );
const { handleSetLibraryFontSelected } = useContext( FontLibraryContext );
const { openModal } = useDispatch( interfaceStore );

const variantsCount = font?.fontFace?.length || 1;

const handleClick = () => {
handleSetLibraryFontSelected( font );
toggleModal( 'installed-fonts' );
openModal( FONT_LIBRARY_MODAL_NAME );
};

const previewStyle = getFamilyPreviewStyle( font );
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { search, closeSmall } from '@wordpress/icons';
* Internal dependencies
*/
import TabPanelLayout from './tab-panel-layout';
import { FontLibraryContext } from './context';
import { FontLibraryContext } from '../font-library-provider';
import FontsGrid from './fonts-grid';
import FontCard from './font-card';
import filterFonts from './utils/filter-fonts';
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/font-library-modal/font-demo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { useContext, useEffect, useState, useRef } from '@wordpress/element';
/**
* Internal dependencies
*/
import { FontLibraryContext } from './context';
import { FontLibraryContext } from '../font-library-provider';
import { getFacePreviewStyle } from './utils/preview-styles';

function getPreviewUrl( fontFace ) {
Expand Down
26 changes: 17 additions & 9 deletions packages/edit-site/src/components/global-styles/font-library-modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,20 @@ import {
privateApis as componentsPrivateApis,
} from '@wordpress/components';
import { useContext } from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { store as interfaceStore } from '@wordpress/interface';

/**
* Internal dependencies
*/
import InstalledFonts from './installed-fonts';
import FontCollection from './font-collection';
import UploadFonts from './upload-fonts';
import { FontLibraryContext } from './context';
import { FontLibraryContext } from '../font-library-provider';
import { unlock } from '../../../lock-unlock';

export const FONT_LIBRARY_MODAL_NAME = 'edit-site/font-library';

const { Tabs } = unlock( componentsPrivateApis );

const DEFAULT_TABS = [
Expand All @@ -39,12 +43,18 @@ const tabsFromCollections = ( collections ) =>
: name,
} ) );

function FontLibraryModal( {
onRequestClose,
initialTabId = 'installed-fonts',
} ) {
export default function FontLibraryModal() {
const { collections, setNotice } = useContext( FontLibraryContext );

const isModalActive = useSelect( ( select ) =>
select( interfaceStore ).isModalActive( FONT_LIBRARY_MODAL_NAME )
);
const { closeModal } = useDispatch( interfaceStore );

if ( ! isModalActive ) {
return null;
}

const tabs = [
...DEFAULT_TABS,
...tabsFromCollections( collections || [] ),
Expand All @@ -58,12 +68,12 @@ function FontLibraryModal( {
return (
<Modal
title={ __( 'Fonts' ) }
onRequestClose={ onRequestClose }
onRequestClose={ closeModal }
isFullScreen
className="font-library-modal"
>
<div className="font-library-modal__tabs">
<Tabs initialTabId={ initialTabId } onSelect={ onSelect }>
<Tabs onSelect={ onSelect }>
<Tabs.TabList>
{ tabs.map( ( { id, title } ) => (
<Tabs.Tab key={ id } tabId={ id }>
Expand Down Expand Up @@ -98,5 +108,3 @@ function FontLibraryModal( {
</Modal>
);
}

export default FontLibraryModal;
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/font-library-modal/installed-fonts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
* Internal dependencies
*/
import TabPanelLayout from './tab-panel-layout';
import { FontLibraryContext } from './context';
import { FontLibraryContext } from '../font-library-provider';
import FontsGrid from './fonts-grid';
import LibraryFontDetails from './library-font-details';
import LibraryFontCard from './library-font-card';
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/font-library-modal/library-font-card.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { useContext } from '@wordpress/element';
* Internal dependencies
*/
import FontCard from './font-card';
import { FontLibraryContext } from './context';
import { FontLibraryContext } from '../font-library-provider';

function LibraryFontCard( { font, ...props } ) {
const { getFontFacesActivated } = useContext( FontLibraryContext );
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/font-library-modal/library-font-variant.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
* Internal dependencies
*/
import { getFontFaceVariantName } from './utils';
import { FontLibraryContext } from './context';
import { FontLibraryContext } from '../font-library-provider';
import FontFaceDemo from './font-demo';
import { unlock } from '../../../lock-unlock';

Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/font-library-modal/upload-fonts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { useContext, useState } from '@wordpress/element';
* Internal dependencies
*/
import { ALLOWED_FILE_EXTENSIONS } from './utils/constants';
import { FontLibraryContext } from './context';
import { FontLibraryContext } from '../font-library-provider';
import { Font } from '../../../../lib/lib-font.browser';
import makeFamiliesFromFaces from './utils/make-families-from-faces';
import { loadFontFaceInBrowser } from './utils';
Expand Down
21 changes: 4 additions & 17 deletions ...obal-styles/font-library-modal/context.js → ...ts/global-styles/font-library-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import {
fetchUninstallFontFamily,
fetchFontCollections,
fetchFontCollection,
} from './resolvers';
import { unlock } from '../../../lock-unlock';
} from './font-library-modal/resolvers';
import { unlock } from '../../lock-unlock';
const { useGlobalSetting } = unlock( blockEditorPrivateApis );
import {
setUIValuesNeeded,
Expand All @@ -32,8 +32,8 @@ import {
makeFontFamilyFormData,
batchInstallFontFaces,
checkFontFaceInstalled,
} from './utils';
import { toggleFont } from './utils/toggleFont';
} from './font-library-modal/utils';
import { toggleFont } from './font-library-modal/utils/toggleFont';

export const FontLibraryContext = createContext( {} );

Expand Down Expand Up @@ -110,7 +110,6 @@ function FontLibraryProvider( { children } ) {
};

// Library Fonts
const [ modalTabOpen, setModalTabOpen ] = useState( false );
const [ libraryFontSelected, setLibraryFontSelected ] = useState( null );

const baseThemeFonts = baseFontFamilies?.theme
Expand All @@ -137,12 +136,6 @@ function FontLibraryProvider( { children } ) {
.sort( ( a, b ) => a.name.localeCompare( b.name ) )
: [];

useEffect( () => {
if ( ! modalTabOpen ) {
setLibraryFontSelected( null );
}
}, [ modalTabOpen ] );

const handleSetLibraryFontSelected = ( font ) => {
setNotice( null );

Expand All @@ -164,10 +157,6 @@ function FontLibraryProvider( { children } ) {
} );
};

const toggleModal = ( tabName ) => {
setModalTabOpen( tabName || null );
};

// Demo
const [ loadedFontUrls ] = useState( new Set() );

Expand Down Expand Up @@ -480,8 +469,6 @@ function FontLibraryProvider( { children } ) {
uninstallFontFamily,
toggleActivateFont,
getAvailableFontsOutline,
modalTabOpen,
toggleModal,
refreshLibrary,
notice,
setNotice,
Expand Down
2 changes: 2 additions & 0 deletions packages/edit-site/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import { useCommonCommands } from '../../hooks/commands/use-common-commands';
import { useEditModeCommands } from '../../hooks/commands/use-edit-mode-commands';
import { useIsSiteEditorLoading } from './hooks';
import useLayoutAreas from './router';
import FontLibraryModal from '../global-styles/font-library-modal';

const { useCommands } = unlock( coreCommandsPrivateApis );
const { useCommandContext } = unlock( commandsPrivateApis );
Expand Down Expand Up @@ -159,6 +160,7 @@ export default function Layout() {
return (
<>
<CommandMenu />
<FontLibraryModal />
<KeyboardShortcutsRegister />
<KeyboardShortcutsGlobal />
{ fullResizer }
Expand Down
Loading