-
Notifications
You must be signed in to change notification settings - Fork 622
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
feat: Icon picker component (WIP) #3487
base: next
Are you sure you want to change the base?
Conversation
@Pavel910 @SvenAlHamad I have a couple questions about Icon Picker:
|
@Pavel910 @SvenAlHamad |
const renderCell = useCallback(() => { | ||
return function renderCell({ | ||
columnIndex, | ||
key, | ||
rowIndex, | ||
style | ||
}: GridCellProps): React.ReactNode { | ||
const item = filteredIcons[rowIndex * COLUMN_COUNT + columnIndex]; | ||
if (!item) { | ||
return null; | ||
} | ||
|
||
return ( | ||
<Cell | ||
key={key} | ||
style={style} | ||
onClick={() => { | ||
onChange({ | ||
type: item.type, | ||
name: item.name, | ||
...(item.type === "emoji" ? { skinTone: item.skinTone } : {}), | ||
...(item.type === "icon" ? { color } : {}), | ||
...(item.width ? { width: item.width } : {}), | ||
value: item.value | ||
}); | ||
}} | ||
color={color} | ||
isActive={item.name === value.name} | ||
> | ||
<IconRenderer icon={item} size={32} /> | ||
</Cell> | ||
); | ||
}; | ||
}, [filteredIcons, color]); |
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.
I think this is overcomplicated; Why return a new function? Further down in JSX, you just call renderCell()
when passing it to props, so you can essentially just do this:
const renderCell = useCallback(({ columnIndex, key, rowIndex }) => { .... }, [filteredIcons, color]);
And then you pass renderCell
as a cellRenderer
prop without invoking it.
const emojis = icons.filter(icon => icon.type === "emoji"); | ||
const defaultIcons = icons.filter(icon => icon.type === "icon"); |
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.
This is something you want to memoize with useMemo
.
}; | ||
|
||
export const IconPackProvider = ({ provider }: IconPackProviderProps) => { | ||
const icons = provider(); |
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.
Can we make this work with async providers? Think code splitting... Would be nice to allow icons to be lazy loaded, or even loaded from an API. That way they'll only load when IconPicker is actually being used in the UI.
I was testing the icon component implementation and have a few UX related points that require a bit of polish:
|
Co-authored-by: Pavel Denisjuk <[email protected]> Co-authored-by: Leonardo Giacone <[email protected]>
Attaching this git patch file here. This contains all changes from the iconpicker PR in one place, so it's easier to integrate with the latest branch. Merging of this is not possible, so we need to carefully pick changes. Part of these changes will be merged in one of the upcoming PRs, but I'm still keeping this here, not to lose it accidentally. |
Changes
Implements issue "Icon picker component".
Implementation steps
Most of the implementation is described here.
For custom icon support IconPickerConfig was added under
packages/app-admin/src/components/IconPicker/config/index.tsx
.For the most part it is similar to what we've done with audit logs here.
It allows to add custom icons/emojis inside DefaultIcons. Also there is an example of how it can be used.
UI
Color Picker UI is currently only available on
/page-builder/block-categories
page for test purposes.How Has This Been Tested?
Manual
Documentation
None