webiny-js
webiny-js copied to clipboard
refactor: replace Icon Picker
Changes
- Added
size
prop toIconPicker
and styles for the smaller version (for PB sidebar). - Added
size
prop toIconRenderer
. - Added ability to remove icons (controllable by
removable
prop). - Added
align
prop toColorPicker
to correctly display it inside PB sidebarIconPicker
. - Replaced old
IconPicker
andIconRenderer
with new ones. -
api-page-builder
andapi-headless-cms
changes (according to new icon value type). - Fixed API tests (replace icon strings with objects).
- Replace icon rendering within the page content.
How Has This Been Tested?
Manual
Documentation
None
@Pavel910, I have a question regarding the rendering of icons within page content.
Previously, we utilized the updateButtonElementIcon
and updateIconElement
functions to generate and insert static markup for icon SVGs directly into icons using the svg
prop. Should we implement a similar approach in our new implementation?
Currently, icon rendering is handled by the <IconPicker.Icon />
component, which renders icons based on their type. We could consider reusing the existing approach of generating static markup with the renderToStaticMarkup
function, this time passing in our new icon rendering method. However, this approach seems to be challenging. Despite being wrapped with the necessary providers, I haven't been successful in making it work. The issue might be related to the malfunctioning behavior of composable components within renderToStaticMarkup
.
Should I investigate this solution further, or do you have any other suggestions for how we might implement this?
@adrians5j, maybe you can suggest smth about how to target this issue?
After further investigation, I've realized that even if we manage to render the element correctly by type within renderToStaticMarkup
, this approach still isn't viable because it fails to include styles from @emotion/styled
. We only receive inline styles, which worked for SVGs but aren't enough for more complex icons.
Since I couldn't make renderToStaticMarkup
work with composable components, I tried different approaches to obtain static HTML. One of them involved placing <IconPicker.Icon />
inside an invisible element within the Admin app and saving its innerHTML
. This approach worked well, and I even set up state updates based on changes in the element's innerHTML
. But the issue with @emotion/styled
styles is still present.
We can consider using inline styles exclusively within our icon renderer plugins, but I'm uncertain if this is a suitable choice.
@Pavel910, @adrians5j, maybe you have some ideas on how we can extract plain HTML for icons, and whether we should do this in the first place?
@neatbyte-vnobis That's a very interesting problem. Looking at the code of our icon types, and how they use emotion, I think we can remove the use of emotion entirely. For regular icons, it will be SVG, just like it was in the original implementation. For emojis, it will be a div, and the styles for EmojiStyled
we can simply define through the style
prop of the div element, and not emotion. For custom icons, it will simply be an <img>
. So I think your approach with generating a static HTML for those elements should work, if we eliminate emotion, which we can.
/cypress
Cypress E2E tests have been initiated (for more information, click here). :sparkles:
@Pavel910, when running Cypress tests, I encountered an issue with the New block category
form. This form requires an icon field. Previously, fas/star
was set as the default value for that field. However, since we've transitioned from using icons as strings to objects, I'm unsure how to handle this field. There are a few possible solutions:
- Leave the field without a default value. This would require updating the Cypress test to select an icon from the
IconPicker
. - Set a default value as an object. Below are some examples of how this might look in the code:
<Bind
name="icon"
validators={validation.create("required")}
defaultValue={{
type: "emoji",
name: "thumbs_up",
value: "👍",
category: "People & Body",
skinToneSupport: true,
skinTone: "🏼"
}}
>
<IconPicker
label={t`Category icon`}
description={t`Icon that will be displayed in the page builder.`}
/>
</Bind>
Alternatively, for an icon similar to the old fas/star
:
<Bind
name="icon"
validators={validation.create("required")}
defaultValue={{
type: "icon",
name: "regular_star",
value: '<path fill="currentColor" d="M287.9 0c9.2 0 17.6 5.2 21.6 13.5l68.6 141.3l153.2 22.6c9 1.3 16.5 7.6 19.3 16.3s.5 18.1-5.9 24.5L433.6 328.4L459.8 484c1.5 9-2.2 18.1-9.6 23.5s-17.3 6-25.3 1.7l-137-73.2L151 509.1c-8.1 4.3-17.9 3.7-25.3-1.7s-11.2-14.5-9.7-23.5l26.2-155.6L31.1 218.2c-6.5-6.4-8.7-15.9-5.9-24.5s10.3-14.9 19.3-16.3l153.2-22.6l68.6-141.3C270.4 5.2 278.7 0 287.9 0zm0 79l-52.5 108.2c-3.5 7.1-10.2 12.1-18.1 13.3L99 217.9l85.9 85.1c5.5 5.5 8.1 13.3 6.8 21l-20.3 119.7l105.2-56.2c7.1-3.8 15.6-3.8 22.6 0l105.2 56.2l-20.2-119.6c-1.3-7.7 1.2-15.5 6.8-21l85.9-85.1l-118.3-17.5c-7.8-1.2-14.6-6.1-18.1-13.3L287.9 79z"/>',
width: 576
}}
>
<IconPicker
label={t`Category icon`}
description={t`Icon that will be displayed in the page builder.`}
/>
</Bind>;
The object-based approach, particularly with the full SVG value, is not very readable. Maybe we can implement default value handling inside the IconPicker
? This way, we could set the default value as either a string or an object with type
and name
, and let the IconPicker
retrieve the value from the config. What are your thoughts on this?
@Pavel910 CC: @SvenAlHamad Please take a look on last question - it is literately last thing we need to resolve to finish this task. Hope to hear you comment soon.
@neatbyte-vnobis Let's set the default value in form of an object. Readability doesn't bother me in this case, because icons are not frequently used via code. You can always extract the icon object into a separate file, to solve the readability issue. Mapping to a string.... I don't know, I'd rather keep the input and output value in the same shape. Let's keep it an object for now.
/cypress
Cypress E2E tests have been initiated (for more information, click here). :sparkles: