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.

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

refactor: replace Icon Picker #3814

Open
wants to merge 9 commits into
base: neatbyte/icon-picker
Choose a base branch
from

Conversation

neatbyte-vnobis
Copy link
Collaborator

@neatbyte-vnobis neatbyte-vnobis commented Jan 12, 2024

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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?

@Pavel910
Copy link
Collaborator

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

@neatbyte-vnobis
Copy link
Collaborator Author

/cypress

Copy link

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

@neatbyte-vnobis
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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

@Pavel910
Copy link
Collaborator

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

@neatbyte-vnobis
Copy link
Collaborator Author

/cypress

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants