webiny-js icon indicating copy to clipboard operation
webiny-js copied to clipboard

refactor: replace Icon Picker

Open neatbyte-vnobis opened this issue 1 year ago • 10 comments

Changes

  1. Added size prop to IconPicker and styles for the smaller version (for PB sidebar).
  2. Added size prop to IconRenderer.
  3. Added ability to remove icons (controllable by removable prop).
  4. Added align prop to ColorPicker to correctly display it inside PB sidebar IconPicker.
  5. Replaced old IconPicker and IconRenderer with new ones.
  6. api-page-builder and api-headless-cms changes (according to new icon value type).
  7. Fixed API tests (replace icon strings with objects).
  8. Replace icon rendering within the page content.

How Has This Been Tested?

Manual

Documentation

None

neatbyte-vnobis avatar Jan 12 '24 16:01 neatbyte-vnobis

@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?

neatbyte-vnobis avatar Jan 12 '24 16:01 neatbyte-vnobis

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 avatar Jan 15 '24 15:01 neatbyte-vnobis

@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.

Pavel910 avatar Jan 15 '24 19:01 Pavel910

/cypress

neatbyte-vnobis avatar Jan 17 '24 11:01 neatbyte-vnobis

Cypress E2E tests have been initiated (for more information, click here). :sparkles:

github-actions[bot] avatar Jan 17 '24 11:01 github-actions[bot]

@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:

  1. Leave the field without a default value. This would require updating the Cypress test to select an icon from the IconPicker.
  2. 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?

neatbyte-vnobis avatar Jan 17 '24 16:01 neatbyte-vnobis

@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 avatar Jan 18 '24 14:01 neatbyte-vnobis

@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.

Pavel910 avatar Jan 18 '24 14:01 Pavel910

/cypress

neatbyte-vnobis avatar Jan 18 '24 16:01 neatbyte-vnobis

Cypress E2E tests have been initiated (for more information, click here). :sparkles:

github-actions[bot] avatar Jan 18 '24 16:01 github-actions[bot]