Repository navigation
Icons: Filter the icon library picker by collection #79681
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.
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
Changes from 4 commits
9402eb0
d6c4fbe
3095b01
edc2d51
edb04bb
deab37a
99e9dd1
09e492b
0988dc6
6d3af21
843a235
8ffcf30
16bd33c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,44 @@ | |
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { Button } from '@wordpress/components'; | ||
| import { useAsyncList } from '@wordpress/compose'; | ||
| import { useRef, useLayoutEffect } from '@wordpress/element'; | ||
| import { getScrollContainer } from '@wordpress/dom'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import HtmlRenderer from '../../../utils/html-renderer'; | ||
|
|
||
| export default function IconGrid( { icons, onChange, attributes } ) { | ||
| const INITIAL_RESULTS = 20; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rename this; I expected to see actual results, not number of results.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find BATCH_SIZE to be the most intuitive. Fixed in 99e9dd1 |
||
|
|
||
| export default function IconGrid( { icons, onChange, value } ) { | ||
| const shownIcons = useAsyncList( icons, { | ||
| step: INITIAL_RESULTS, | ||
| } ); | ||
|
|
||
| // Scroll the selected icon into view, but wait until enough icons render | ||
| // below it so it can be centered rather than stuck at the bottom. Skip it | ||
| // if the user has already scrolled the list. | ||
| const selectedIconRef = useRef(); | ||
| const selectedIndex = | ||
| icons?.findIndex( ( icon ) => icon.name === value ) ?? -1; | ||
| const isReadyToScroll = | ||
| selectedIndex >= 0 && | ||
| ( shownIcons.length >= selectedIndex + INITIAL_RESULTS || | ||
| shownIcons.length === icons.length ); | ||
|
|
||
| useLayoutEffect( () => { | ||
| const node = selectedIconRef.current; | ||
| if ( ! isReadyToScroll || ! node ) { | ||
| return; | ||
| } | ||
| if ( getScrollContainer( node )?.scrollTop ) { | ||
| return; | ||
| } | ||
| node.scrollIntoView( { block: 'center' } ); | ||
| }, [ isReadyToScroll ] ); | ||
|
|
||
| return ( | ||
| <div className="wp-block-icon__inserter-grid"> | ||
| { ! icons?.length ? ( | ||
|
|
@@ -21,16 +52,19 @@ export default function IconGrid( { icons, onChange, attributes } ) { | |
| className="wp-block-icon__inserter-grid-icons-list" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, the current implementation only applies a 24px width to the icon, so if the SVG icon itself has a
I think we can fix this in this PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| aria-label={ __( 'Icon library' ) } | ||
| > | ||
| { icons.map( ( icon ) => { | ||
| { shownIcons.map( ( icon ) => { | ||
| return ( | ||
| <Button | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also preexisting, but I guess this should ideally use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's right. I would like to consider addressing this in a follow-up. Another idea is to completely refactor this icon list using |
||
| key={ icon.name } | ||
| ref={ | ||
| icon.name === value | ||
| ? selectedIconRef | ||
| : undefined | ||
| } | ||
| className="wp-block-icon__inserter-grid-icons-list-item" | ||
| onClick={ () => onChange( icon.name ) } | ||
| variant={ | ||
| icon.name === attributes?.icon | ||
| ? 'primary' | ||
| : undefined | ||
| icon.name === value ? 'primary' : undefined | ||
| } | ||
| __next40pxDefaultSize | ||
| > | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,37 +2,68 @@ | |
| * WordPress dependencies | ||
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { Modal, SearchControl } from '@wordpress/components'; | ||
| import { useState, useMemo, useCallback } from '@wordpress/element'; | ||
| import { Modal, SearchControl, Spinner } from '@wordpress/components'; | ||
| import { Stack, Tabs } from '@wordpress/ui'; | ||
| import { useState, useMemo } from '@wordpress/element'; | ||
| import { useDebounce } from '@wordpress/compose'; | ||
| import { useSelect } from '@wordpress/data'; | ||
| import { store as coreDataStore } from '@wordpress/core-data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import IconGrid from './icon-grid'; | ||
| import { normalizeSearchInput } from '../../../utils/search-patterns'; | ||
|
|
||
| export default function CustomInserterModal( { | ||
| icons = [], | ||
| setInserterOpen, | ||
| attributes, | ||
| setAttributes, | ||
| } ) { | ||
| export default function CustomInserterModal( { onClose, value, onChange } ) { | ||
| const [ searchInput, setSearchInput ] = useState( '' ); | ||
| const [ currentCollection, setCurrentCollection ] = useState( null ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we choose to keep the search within the current collection only, should we reset
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added the "All" tab and made the search function operate globally.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the search behavior changes depending on the selected tab, a common pattern to make things clearer is to place the search field within the tabpanel. Not a blocker for merge, and not sure if there are similar patterns within the app. But ideally, I think the search field placement should be clearer.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| const debouncedSetSearchInput = useDebounce( setSearchInput, 300 ); | ||
|
|
||
| const setIcon = useCallback( | ||
| ( name ) => { | ||
| setAttributes( { | ||
| icon: name, | ||
| } ); | ||
| setInserterOpen( false ); | ||
| const collections = useSelect( | ||
| ( select ) => | ||
| select( coreDataStore ).getEntityRecords( | ||
| 'root', | ||
| 'iconCollection' | ||
| ), | ||
| [] | ||
| ); | ||
|
|
||
| // Default to the collection the selected icon belongs to, otherwise the | ||
| // first collection. | ||
| const selectedCollection = value?.split( '/' )[ 0 ]; | ||
| const collectionSlug = | ||
| currentCollection ?? | ||
| ( collections?.some( ( { slug } ) => slug === selectedCollection ) | ||
| ? selectedCollection | ||
| : collections?.[ 0 ]?.slug ) ?? | ||
| null; | ||
|
|
||
| const { icons, hasResolvedIcons } = useSelect( | ||
| ( select ) => { | ||
| if ( ! collectionSlug ) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WIll this ever happen?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. This may be |
||
| return { icons: null, hasResolvedIcons: false }; | ||
| } | ||
| const query = { namespace: collectionSlug }; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, the fact that the search will now work only within the selected collection feels like a regression. Even in the inserter, search will work across all collections. Is there a way for the search to work across collections? Or is there a good reason why we don't want it to work that way?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added the "All" tab and made the search function operate globally. |
||
| const { getEntityRecords, hasFinishedResolution } = | ||
| select( coreDataStore ); | ||
| return { | ||
| icons: getEntityRecords( 'root', 'icon', query ), | ||
| hasResolvedIcons: hasFinishedResolution( 'getEntityRecords', [ | ||
| 'root', | ||
| 'icon', | ||
| query, | ||
| ] ), | ||
| }; | ||
| }, | ||
| [ setAttributes, setInserterOpen ] | ||
| [ collectionSlug ] | ||
| ); | ||
|
|
||
| const filteredIcons = useMemo( () => { | ||
| if ( ! icons ) { | ||
| return []; | ||
| } | ||
| if ( searchInput ) { | ||
| const input = normalizeSearchInput( searchInput ); | ||
| return icons.filter( ( icon ) => { | ||
|
|
@@ -52,22 +83,55 @@ export default function CustomInserterModal( { | |
| <Modal | ||
| className="wp-block-icon__inserter-modal" | ||
| title={ __( 'Icon library' ) } | ||
| onRequestClose={ () => setInserterOpen( false ) } | ||
| onRequestClose={ onClose } | ||
| isFullScreen | ||
| > | ||
| <div className="wp-block-icon__inserter"> | ||
| <div className="wp-block-icon__inserter-header"> | ||
| <Tabs.Root | ||
| className="wp-block-icon__inserter" | ||
| orientation="vertical" | ||
| value={ collectionSlug } | ||
| onValueChange={ setCurrentCollection } | ||
| > | ||
| <Stack | ||
| direction="column" | ||
| gap="lg" | ||
| className="wp-block-icon__inserter-sidebar" | ||
| > | ||
| <SearchControl | ||
| value={ searchInput } | ||
| onChange={ debouncedSetSearchInput } | ||
| /> | ||
| </div> | ||
| <IconGrid | ||
| icons={ filteredIcons } | ||
| onChange={ setIcon } | ||
| attributes={ attributes } | ||
| /> | ||
| </div> | ||
| <Tabs.List className="wp-block-icon__inserter-collections"> | ||
|
t-hamano marked this conversation as resolved.
Resolved
|
||
| { collections?.map( ( collection ) => ( | ||
| <Tabs.Tab | ||
| key={ collection.slug } | ||
| value={ collection.slug } | ||
| > | ||
| { collection.label } | ||
| </Tabs.Tab> | ||
| ) ) } | ||
| </Tabs.List> | ||
| </Stack> | ||
| { collections?.map( ( collection ) => ( | ||
| <Tabs.Panel | ||
| key={ collection.slug } | ||
| value={ collection.slug } | ||
| className="wp-block-icon__inserter-panel" | ||
| > | ||
| { ! hasResolvedIcons ? ( | ||
| <div className="wp-block-icon__inserter-loading"> | ||
| <Spinner /> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likely needs an accessible label
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch, fixed in 09e492b |
||
| </div> | ||
| ) : ( | ||
| <IconGrid | ||
| icons={ filteredIcons } | ||
| onChange={ onChange } | ||
| value={ value } | ||
| /> | ||
| ) } | ||
| </Tabs.Panel> | ||
| ) ) } | ||
| </Tabs.Root> | ||
| </Modal> | ||
| ); | ||
| } | ||




There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The larger this value is, the faster icons load, but the more the UI tends to freeze.