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

FEATURE: Introduce icon configuration for nodetypes #3042

Merged

Conversation

markusguenther
Copy link
Member

@markusguenther markusguenther commented Feb 25, 2022

What I did
Added a new option largeIcon and the node type item component checks if we configure a larger icon and render this instead if the regular icon. The larger icon also uses the doubled size if we don't configure a largeIconSize, which allows values like ['xs', 'sm', 'lg', '2x', '3x'].

The regular icon will be used in the trees, sidebars and so on.
The large icon is detailed and for the dialog with the tiles and as the detailed icon probably look inappropriate for the tree and so on we use the regular icon here.

How to verify it

Configure a node type with the property largeIcon and check the select node type dialog after that.

'Neos.Demo:Content.Text':
  ui:
    icon: 'icon-file-text'
    largeIcon: 'resource://Neos.Demo/Images/customIcon.svg'
    largeIconSize: '3x'

Screenshot 2022-02-25 at 18 48 22

resolves: #3041

const label = $get('label', ui);
const useLargeIcon = ('largeIcon' in ui);
Copy link
Member

@jonnitto jonnitto Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between ('largeIcon' in ui); and $get('icon', ui)? Why not use the same method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks if we have the largeIcon property in the options.
And then we use the icon or the largeIcon

@jonnitto
Copy link
Member

What I'm missing is the documentation part…

@markusguenther
Copy link
Member Author

@jonnitto We have no documentation in Code... so when we merge this we need to adjust the docs in the neos/neos package.

@markusguenther markusguenther added Feature Label to mark the change as feature 8.0 labels Feb 28, 2022
@markusguenther
Copy link
Member Author

Question: I did not use all possible scaling values from font-awesome because I thought it looks ugly when you scale higher than 3 times. And they support up to 10x.

Is that ok? Or should we also support the others.

@Sebobo
Copy link
Member

Sebobo commented Feb 28, 2022

Then we should call the parameter not largeIcon but directly one of XS, sm, etc... right?
And the component decides which to use, with a fallback mechanism. Wdyt?

@markusguenther
Copy link
Member Author

Then we should call the parameter not largeIcon

TBH, I had also issued to name the property. My goal was to have an extra icon for the select node dialog and choose largeIcon caused by the bigger representation in the view. But prefixing the icon with the size sounds not so nice.

As we have an 8.0 we can maybe change the whole naming. Make something like primaryIcon and secondaryIcon and for these settings I can imagine something like:

primaryIcon:
    size: '2x'
    icon: 'resource://Neos.Demo/Images/customIcon.svg'

We, of course need to keep the old icon for compatibility reasons, as we did not deprecate the icon before.
Perhaps we can also provide a migration. Question is if we want to handle the sizing as well for the secondaryIcon as it is used for the tree and sidebar.

@Sebobo
Copy link
Member

Sebobo commented Mar 1, 2022

then how about introducing icon-lg instead of largeIcon and leave the rest as you did. Then we could extend it when needed

@markusguenther
Copy link
Member Author

So, just icon-lg and no extra size? Or keep the idea of splitting the icon and size? So we have icon-2x, icon-3x and so on?

@mhsdesign
Copy link
Member

As we have an 8.0 we can maybe change the whole naming. Make something like primaryIcon and secondaryIcon and for these settings I can imagine something like:

i like that the 'normal' config for icon is flat like ui.icon: 'icon-file-text' as one usually doesnt need to configure much there.

the name largeIcon doesnt really fit into this maybe icon-lg really is better.

I at first thought the largeIcon would in any case overrule the normal icon - maybe name the property so that this is more intuitive, but i cant think of anything fitting ...

@mficzel
Copy link
Member

mficzel commented Mar 15, 2022

How about naming the config key "preview" and use this for the creation dialog with a fallback to "icon" if no preview exists.

@Sebobo
Copy link
Member

Sebobo commented Mar 18, 2022

How about naming the config key "preview" and use this for the creation dialog with a fallback to "icon" if no preview exists.

That would have the advantage that the integrator would know what they are defining in yaml.
Only setting various sizes and not really understanding where they are used doesn't help so much.

@markusguenther
Copy link
Member Author

Ok so we use the preview key and drop the ability to adjust the scaling?
Because I also like that the integrators are able to upscale the icon in the select dialog a bit.

@mficzel
Copy link
Member

mficzel commented Mar 22, 2022

Maybe display previews larger and allow icon names in there aswell as svg resources. That way we get away with a single configuration option.

@markusguenther
Copy link
Member Author

Hmm the icon name and SVG is already possible, but the in the dialog the scaling is at the moment always 2x.

With SVG the scaling is not adjustable. Therefore the idea to have a second configuration for the size.

@jonnitto
Copy link
Member

So the name would be previewIconSize, right?

@mficzel
Copy link
Member

mficzel commented Mar 22, 2022

Would leave the size setting for now and just assume that previews are larger than icons. If no preview is configured the previous size is used. If a preview is set a bigger size is used even if the preview is an icon.

@markusguenther
Copy link
Member Author

But having a bigger size for the preview is maybe irritating to the users. But if this is not the most wanted feature I will skip that and start with the extra preview icon 🙃

@jonnitto
Copy link
Member

I would like to have the option for the size. I would have a current use case for this…

@mficzel
Copy link
Member

mficzel commented Mar 22, 2022

Fine if there is an actual useCase ... i was just against adding things that might be useful one day.

If you use custom icons, you want to use detailed icons, which describes the use case better, in the node type model. But then the detailed icon is not the best choice for the document or content tree.

So, this feature will bring a new configuration to have a different icon for the node type dialog tiles. We can now configure a icon and a largeIcon setting for the node type.
To be able to configure the scaling of the largeIcon option we also introduce the largeIconSize.
@markusguenther markusguenther force-pushed the feature/largeIcon-for-create-dialog branch from ed91154 to 3f97635 Compare March 23, 2022 09:50
@markusguenther
Copy link
Member Author

Renamed it now so we have something like this.

'Neos.Demo:Content.Text':
  ui:
    icon: 'icon-file-text'
    preview: 'resource://Neos.Demo/Images/customIcon.svg'
    previewIconSize: '3x'

@jonnitto
Copy link
Member

Naming things…
Just a question (Sorry for that):

Why it is called preview when the size setting is previewIconSize? Shouldn't it be named previewIcon?

@markusguenther
Copy link
Member Author

Can also name it like this. Named it preview as it was a suggestion and had two likes ;)

@markusguenther
Copy link
Member Author

So now we have something like this :)

'Neos.Demo:Content.Text':
  ui:
    icon: 'icon-file-text'
    previewIcon: 'resource://Neos.Demo/Images/customIcon.svg'
    previewIconSize: '3x'

@markusguenther markusguenther merged commit 20c88da into neos:master Mar 23, 2022
@markusguenther
Copy link
Member Author

Thanks for the feedback and reviews 💙

markusguenther added a commit to markusguenther/neos-development-collection that referenced this pull request Mar 23, 2022
The neos-ui introduces a new NodeType setting called previewIcon and previewIconSize.
As the neos-ui does not contain documentation, the PR will add the new options here.

The change also adjust the icon description, as we are now able to use custom SVG icons.

Related PR: neos/neos-ui#3042
neos-bot pushed a commit to neos/neos that referenced this pull request Mar 25, 2022
The neos-ui introduces a new NodeType setting called previewIcon and previewIconSize.
As the neos-ui does not contain documentation, the PR will add the new options here.

The change also adjust the icon description, as we are now able to use custom SVG icons.

Related PR: neos/neos-ui#3042
neos-bot pushed a commit to neos/neos that referenced this pull request Oct 7, 2022
The neos-ui introduces a new NodeType setting called previewIcon and previewIconSize.
As the neos-ui does not contain documentation, the PR will add the new options here.

The change also adjust the icon description, as we are now able to use custom SVG icons.

Related PR: neos/neos-ui#3042
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.0 Feature Label to mark the change as feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce second icon property for larger or more detailed icons
5 participants