feat: icon only buttons #4642
feat: icon only buttons #4642
Conversation
- button can be set as icon only using a prop - the btn label is used as default tooltip - additional tooltip can be set with tooltip property
connoratrug
requested review from
davidruvolo51,
mswertz,
chinook25 and
MaxPostema
There was a problem hiding this comment.
you might instead of iconOnly check for an empty slot?
https://mokkapps.de/vue-tips/check-if-slot-is-empty
Otherwise looks great so if you don't think this is possible/better please merge as is.
| @@ -13,13 +13,16 @@ const props = withDefaults( | |||
| icon?: string; | |||
| iconPosition?: ButtonIconPosition; | |||
| disabled?: boolean; | |||
| iconOnly?: boolean; | |||
There was a problem hiding this comment.
this is not possible automatic by checken if there are child elements?
There was a problem hiding this comment.
otherwise I don't mind for now.
There was a problem hiding this comment.
i tried something like that , but it did not work , i'll have a look if de links contain additional info , i'm not even really sure i like the slot option in the case as it can get a bit weird when passing non string stuff
There was a problem hiding this comment.
You might be able to use UseTemplateRef and evaluate if there is inner html.
There was a problem hiding this comment.
I think it is also possible to use a named slot and check if that has content.
There was a problem hiding this comment.
This is great and it looks nice in the story. I have two minor points.
- I think we can remove the span that wraps the icon. The v-if can be moved the BaseIcon line. (Unless this was needed for another reason).
- I think the label should be required if the icon only option is true. The validation should also flag issues if the label is blank or only spaces. Otherwise, the context is missing for the icon/button.
| @@ -13,13 +13,16 @@ const props = withDefaults( | |||
| icon?: string; | |||
| iconPosition?: ButtonIconPosition; | |||
| disabled?: boolean; | |||
| iconOnly?: boolean; | |||
There was a problem hiding this comment.
You might be able to use UseTemplateRef and evaluate if there is inner html.
| @@ -55,22 +58,27 @@ const colorClasses = computed(() => { | |||
| }); | |||
|
|
|||
| const sizeClasses = computed(() => { | |||
| return SIZE_MAPPING[props.size]; | |||
| return props.iconOnly ? "p-[8px]" : SIZE_MAPPING[props.size]; | |||
There was a problem hiding this comment.
Would it be possible create a mapping for icon only buttons? I can imagine a few instances (in the dashboards) where we want an icon only button, but would like to have a larger button. This option would make the icon only option more useful.
There was a problem hiding this comment.
yes , i initially added it similar to the text sizing , but its not used in the current design, so i'm not sure we need it , propose we keep it simple until there is a need for this , then we can test it in the context the usecase
|
|
||
| const tooltipText = computed(() => { | ||
| return props.tooltip || props.iconOnly ? props.label : ""; | ||
| }); |
There was a problem hiding this comment.
Is there a way to make the label required if the iconOnly prop is true? Alternatively, there could be a check here that throws a warning that the label is missing or blank. Otherwise, the context is lost if the default value for label is "".
There was a problem hiding this comment.
not really as far as i know , but we can trow a warning i guess , i think it would mean adding a watch as the prop can change , so it might not be worth extra run time code
|
What are the main changes you did
feat: icon only buttons
How to test
Checklist