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

docs: fix type annotation for icon slot #6336

Merged
merged 1 commit into from
Jan 23, 2023
Merged

docs: fix type annotation for icon slot #6336

merged 1 commit into from
Jan 23, 2023

Conversation

Lukas742
Copy link
Collaborator

@Lukas742 Lukas742 commented Jan 23, 2023

This PR changes the JSDoc annotation of the icon slot for the Badge, ComboBox and MultiComboBox component.

For the Badge I changed it because the TypeScript type accepts an array of HTMLElements and not only a single element. For the ComboBox and the MultiComboBox I changed it because the type of the Input accepts an array as well and since (at least in my opinion) these components should behave in the same way, they should also accept an array.

Additionally I believe that the TypeScript type of the MessageStrip is wrong. When multiple icons are rendered inside the slot it overflows the text:
image

If you still want me to change the JSDoc type for the MessageStrip, please let me know.

@ilhan007
Copy link
Member

Hello @Lukas742 thanks for the PR. I approve it and merge it.

As for the MessageStrip, I assume you refer to

@slot()
icon!: Array<Icon>;"

the thing is that on framework level, all slots are initialised as an empty array.

If some wants to check if there are icons in the slot, you will need to write "icon.length".
Here the problem is with the name, it should have been "icons" and this is marked as thing to change before a major release
in Deprecated Slots section of this PR

And as a matter of fact, no-one can stop you from adding multiple child in any slot.
In the case of the MessageStrip, it's just not considered and handled, that's why it looks ugly if you add many icons.
But it's not handled because it was also never considered as a use-case, not part of the specs as well.

BR,
ilhan

@ilhan007 ilhan007 merged commit b1e6d5c into main Jan 23, 2023
@ilhan007 ilhan007 deleted the docs/icon-slot-type branch January 23, 2023 16:45
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