FEATURE: Added icon selection #6872

popoplanter · 2022-10-28T13:17:34Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
License	MIT
Documentation PR	sulu/sulu-docs#

What's in this PR?

Adds the possibility to select icons with a new single_icon_select content type, the possibility of multiple icon set providers (icomoon, files),and a simple search to help find the right icon.

Why?

A nice selection of icons and how they look is much more user friendly than previous solutions to manage icons such as having a text_line where the icon name is provided.

Example Usage

Add icon sets:

sulu_admin:
    icon_sets:
        icomoon:
            provider: icomoon
            options:
                selection_json: '%kernel.project_dir%/assets/website/selection.json'
        website:
            provider: svg
            options:
                path: '%kernel.project_dir%/public/icons/'

Use the new content type:

<property name="icon" type="single_icon_select">
    <meta>
        <title lang="de">Icon</title>
        <title lang="en">Icon</title>
    </meta>

    <params>
        <param name="icon_set" value="website"/>
    </params>
</property>

To Do

Create a documentation PR
Add breaking changes to UPGRADE.md

alexander-schranz · 2022-11-02T10:45:19Z

How would currently the configuration of the content type? How and who is loading the selection.json?

To be with the naming consistent we should go with single_icon_select as selection are normally resources selection and there exist no icon resource yet.

We should discuss how we make it possible to have multiple providers not only icomoon. Common usecase would be:

font awesome
tailwinds hero icons (directory just with svg's)
directory of twig icons for inline icon usage

How we handle if the selection.json and other required "provider" files can't be in the public directory maybe we need to stream this via a controller.

alexander-schranz · 2022-11-02T11:59:01Z

Maybe we need some configuration in the sulu_admin for this new field type:

sulu_admin:
   icon_sets:
       website:
           provider: icomoon
           options:
               selection_json: '%kernel.project_dir%/assets/website/selection.json'
       other:
           provider: fontawesome

<property type="single_icon_select">
    <params>
         <param name="icon_set" value="website"/>
    </params>
</property>

To get the information to the frontend we maybe need an additional controller. /admin/api/icons?icon_set=website: IconController.

alexander-schranz · 2022-11-07T13:19:50Z

As discussed I think the best would be that we reuse existing components like the https://github.com/sulu/sulu/blob/2.5/src/Sulu/Bundle/AdminBundle/Resources/js/containers/SingleListOverlay/README.md to fetch the icon resource from the controller. So you don't need todo any fetch / request and reuse existing loading strategies to load the list data for the icons.

For this we mostly need to create our own Adapter for lists via the listRegistry. But lets check what maybe @niklasnatter has as input here.

popoplanter · 2022-11-15T15:47:49Z

@wachterjohannes @luca-rath @alexander-schranz I fixed the checks except for one node lint error that i don't know how to fix. Maybe you know how to fix it? also a code review needs to be made :) thank you

src/Sulu/Bundle/AdminBundle/Resources/js/components/SingleIconSelect/SingleIcon.js

src/Sulu/Bundle/AdminBundle/Resources/js/containers/Form/fields/SingleIconSelect.js

niklasnatter · 2022-11-24T10:18:40Z

src/Sulu/Bundle/AdminBundle/Resources/js/containers/Form/fields/SingleIconSelect.js

+type Props = FieldTypeProps<?string>
+
+@observer
+export default class SingleIconSelect extends React.Component<Props> {


as this is implemented using a list overlay now, i guess it would be more consistent to call it SingleIconSelection (same for the name of the field type and content type). but i am not sure if you already talked about this and if there are good reasons to keep select

also, we should add basic test cases for the component before merging it

@alexander-schranz @niklasnatter we talked about that but i think this was before the implementation with lists... should we use select or selection? Also i would appreciate some help with the test cases...

you dont need to worry about the tests too much. if everything else works, i can add the test cases 🙂

@alexander-schranz what about the naming? select or selection?

niklasnatter · 2022-11-24T10:24:55Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/SingleIconSelect/SingleIcon.js

+    onClick?: (id: string | number, selected: boolean) => void,
+};
+
+export default class SingleIcon extends React.PureComponent<Props> {


as far as i see, this component is not used by the SingleIconSelect component directly (only indirectly via the IconAdapter).

therefore i would prefer to make this consistent to the MediaCardAdapter and call it IconCard and store it in a separate folder (components/IconCard)

also, we should add basic test cases for the component before merging it

src/Sulu/Bundle/AdminBundle/Resources/js/containers/Form/fields/SingleIconSelect.js

src/Sulu/Bundle/AdminBundle/Resources/js/containers/List/adapters/IconAdapter.js

niklasnatter · 2022-11-24T10:46:49Z

src/Sulu/Bundle/AdminBundle/Resources/js/containers/List/adapters/IconAdapter.js

+import AbstractAdapter from './AbstractAdapter';
+
+@observer
+class IconAdapter extends AbstractAdapter {


we need to add basic test cases for the component

niklasnatter · 2022-11-24T10:47:30Z

src/Sulu/Bundle/PageBundle/Controller/IconController.php

+        $icons = [];
+        $path = $iconSet['options']['path'];
+
+        // TODO maybe do this in a compilerpass to avoid file system access in the runtime


what do you think @alexander-schranz?

alexander-schranz · 2023-01-05T15:03:36Z

@tommysonsylverstone I did today stumble over your created project: https://github.com/sevenGroupFrance/sulu-icon-picker-bundle. It would be great to get your feedback about this implementation here if it would also match your usecase.

tommysonsylverstone · 2023-01-09T08:42:09Z

@tommysonsylverstone I did today stumble over your created project: https://github.com/sevenGroupFrance/sulu-icon-picker-bundle. It would be great to get your feedback about this implementation here if it would also match your usecase.

Hello Alex, I'll try using this wip feature asap. Our icon picker is still far from perfect (like, no fontawesome providers like yours, only icoMoon support etc).

popoplanter · 2023-02-15T15:50:11Z

@alexander-schranz will this be merged soon?

alexander-schranz · 2023-02-15T15:58:40Z

@popoplanter I hope to get some feedback first from @tommysonsylverstone and @Webstores /cc @kleinkoerkamp which have similar usecases for there projects. To make sure we are not forgetting something here.

But definitly this is planned to be merged before releasing 2.6.

tommysonsylverstone · 2023-03-06T08:45:31Z

Hello @alexander-schranz I tried installing the feature by pulling the feature/icons branch of popoplanter's repo but it says that single_icon_selection is unrecognized in the admin, same thing happens when modifying the files directly.
If you could tell me how to try the feature you'd be of great help. Thanks.

alexander-schranz · 2023-03-06T09:21:40Z

@tommysonsylverstone It should be there but it is named currently single_icon_select. You can always list all content types via bin/console sulu:content:types:dump. A custom build is required in the project you are trying it out.

0xJas0n · 2023-10-31T13:45:00Z

@alexander-schranz will this be merged soon?

alexander-schranz · 2023-10-31T16:18:36Z

@0xJas0n somebodyshould first rebase it and tackle the failing CI before it can be merged.

martinlagler · 2024-03-13T16:01:40Z

@alexander-schranz I did rebase this branch and fixed the pipeline so far.

alexander-schranz · 2024-04-29T15:24:02Z

Things I think we should change before the merge of this.

A. The current configuration we maybe should be more DSN driven to be more flexible:

sulu_admin:
    icon_sets:
        website: icomoon://%kernel.project_dir%/assets/website/selection.json
        app: svg://%kernel.project_dir%/public/icons/
        more: twig://%kernel.project_dir%/templates/icons/?default-params[width]=w-6&default-params[height]=w-8&default-params[color]=black # maybe some more flexible iconset params

B. Move the Controller

Currently the config is inside sulu_admin but the IconController in the PageBundle think we should move the controller also the the admin bundle.

C. Controller should never access the filesystem

Currently every call of the IconController access the filesystem instead we should prepare a the iconset configuration already in a compiler pass or some cache if we don't want to make the container to big.

D. Naming of the content type

Should we name the content type / field type single_icon_select or single_icon_selection? /cc @sulu/core-developer

E. Add some basic tests

Currently there are not lot of tests for the new PHP or JS code.

martinlagler · 2024-04-30T05:49:02Z

@alexander-schranz I personally would go with single_icon_selection, because we already have single_page_selection, single_media_selection,... And this would be a little confusing for me

Prokyonn · 2024-04-30T06:53:33Z

@alexander-schranz I would also go with the selection, in the UI it feels more like a selection and as far as I know it is not defined, that a selection always has to have an actual entity behind the curtains 😬

alexander-schranz added the Feature New functionality not yet included in Sulu label Nov 2, 2022

alexander-schranz changed the base branch from 2.5 to 2.6 November 11, 2022 09:36

popoplanter force-pushed the feature/icons branch 16 times, most recently from 7b72926 to 9d758ee Compare November 15, 2022 15:27

niklasnatter reviewed Nov 24, 2022

View reviewed changes

popoplanter force-pushed the feature/icons branch 7 times, most recently from 380d8b9 to 3b6a61f Compare November 29, 2022 09:29

popoplanter force-pushed the feature/icons branch from 3b6a61f to 72629e7 Compare November 29, 2022 09:51

alexander-schranz added the UX Affecting the end user label Feb 15, 2023

martinlagler force-pushed the feature/icons branch 2 times, most recently from 331d996 to 50a2bb8 Compare March 13, 2024 15:21

alexander-schranz mentioned this pull request Apr 18, 2024

Feature request: Allow fragments (and query string) for media-links #7082

Closed

popoplanter and others added 8 commits April 29, 2024 17:13

Add icon selection

430a87b

fix pipeline

ded4e96

fix offset on mixed

fbaaf93

fix onclick function

5d83a95

test if docblocks can be removed

eaa86f6

fixed MR

f488491

Fixed icon set content

c262042

Rebase + update baseline

d64e428

alexander-schranz force-pushed the feature/icons branch from 50a2bb8 to d64e428 Compare April 29, 2024 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Added icon selection #6872

FEATURE: Added icon selection #6872

popoplanter commented Oct 28, 2022 •

edited

alexander-schranz commented Nov 2, 2022

alexander-schranz commented Nov 2, 2022 •

edited

alexander-schranz commented Nov 7, 2022

popoplanter commented Nov 15, 2022

niklasnatter Nov 24, 2022

niklasnatter Nov 24, 2022

popoplanter Nov 24, 2022

niklasnatter Nov 24, 2022

popoplanter Nov 29, 2022

niklasnatter Nov 24, 2022

niklasnatter Nov 24, 2022

niklasnatter Nov 24, 2022

niklasnatter Nov 24, 2022

alexander-schranz commented Jan 5, 2023

tommysonsylverstone commented Jan 9, 2023

popoplanter commented Feb 15, 2023

alexander-schranz commented Feb 15, 2023

tommysonsylverstone commented Mar 6, 2023

alexander-schranz commented Mar 6, 2023

0xJas0n commented Oct 31, 2023

alexander-schranz commented Oct 31, 2023

martinlagler commented Mar 13, 2024

alexander-schranz commented Apr 29, 2024 •

edited

martinlagler commented Apr 30, 2024

Prokyonn commented Apr 30, 2024

FEATURE: Added icon selection #6872

Are you sure you want to change the base?

FEATURE: Added icon selection #6872

Conversation

popoplanter commented Oct 28, 2022 • edited

What's in this PR?

Why?

Example Usage

To Do

alexander-schranz commented Nov 2, 2022

alexander-schranz commented Nov 2, 2022 • edited

alexander-schranz commented Nov 7, 2022

popoplanter commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-schranz commented Jan 5, 2023

tommysonsylverstone commented Jan 9, 2023

popoplanter commented Feb 15, 2023

alexander-schranz commented Feb 15, 2023

tommysonsylverstone commented Mar 6, 2023

alexander-schranz commented Mar 6, 2023

0xJas0n commented Oct 31, 2023

alexander-schranz commented Oct 31, 2023

martinlagler commented Mar 13, 2024

alexander-schranz commented Apr 29, 2024 • edited

A. The current configuration we maybe should be more DSN driven to be more flexible:

B. Move the Controller

C. Controller should never access the filesystem

D. Naming of the content type

E. Add some basic tests

martinlagler commented Apr 30, 2024

Prokyonn commented Apr 30, 2024

popoplanter commented Oct 28, 2022 •

edited

alexander-schranz commented Nov 2, 2022 •

edited

alexander-schranz commented Apr 29, 2024 •

edited