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

feat(VBtn): add iconSize prop #16550

Closed
wants to merge 1 commit into from
Closed

feat(VBtn): add iconSize prop #16550

wants to merge 1 commit into from

Conversation

floroz
Copy link
Contributor

@floroz floroz commented Jan 30, 2023

Description

Starting point to discuss the potential implementation of iconSize prop and its behaviour

The problem

Currently, <VIcon /> rendered from the <VBtn /> do not receive any size prop, and therefore they don't scale accordingly to the users' needs.

One potential approach would be to automatically scale with the same sizes provided to the <VBtn />, but it would limit the ability for the user to customize the look for built-in icons.

The alternative approach is to add a new iconSize prop to the <VBtn /> (and potentially replicate this approach to other components that leverage icons?).

Open to thoughts and considerations.

Related issues

closes #16288

@floroz
Copy link
Contributor Author

floroz commented Jan 30, 2023

Currently the implementation breaks the scaling of the button when the icon overflows:

Screenshot 2023-01-30 at 10 45 21

Before solving this issue, I prefer to validate that this approach could be valid/correct, since the Btn implementation has many dependencies and there might be quite a few things I am missing.

I would also like to hear thoughts about potentially using the same props.size provided to the VBtn and also to the VIcon and how it should behave.

@floroz
Copy link
Contributor Author

floroz commented Jan 30, 2023

However, even using the props.size instead of props.iconSize causes issue in correctly generating the layouts

Screenshot 2023-01-30 at 10 52 56
Screenshot 2023-01-30 at 10 53 16

@KaelWD
Copy link
Member

KaelWD commented Jan 30, 2023

That's why #15516 was reverted.

Related: #15746, #15515

Basically the button-sizes sass mixin has to be rewritten to accept arbitrary values from a prop instead of using $size-scales

@floroz
Copy link
Contributor Author

floroz commented Jan 30, 2023

That's why #15516 was reverted.

Related: #15746, #15515

Basically the button-sizes sass mixin has to be rewritten to accept arbitrary values from a prop instead of using $size-scales

Thanks, @KaelWD for the clarification! Is this something still available to work? I am not a SASS expert but can give it a try.

Do we have any capability for Cypress Stories to generate some Visual testing?

This feature seems a good candidate for that.

@floroz
Copy link
Contributor Author

floroz commented Jan 30, 2023

Oh, also to clarify further, @KaelWD this means that props.size should be sufficient to implement the feature and we don't need a props.iconSize for further customization?

@KaelWD
Copy link
Member

KaelWD commented Jan 30, 2023

It's pretty complex with size+density so I don't have high hopes that you'll be able to solve it, you're always welcome to try though.

Do we have any capability for Cypress Stories to generate some Visual testing?

Yes, the last test ("Showcase") is a percy snapshot. I don't think it's publicly accessible but you can add new stories to it and ask me to run them. You can see how it looks at least with yarn cy:open

we don't need a props.iconSize

Correct. Size presets (size="xs") already affect font size too, it's just arbitrary pixel values that don't. The way you'd done it here isn't great anyway because multiple components use the size composable, and not all of them have icons.

@floroz
Copy link
Contributor Author

floroz commented Jan 30, 2023

It's pretty complex with size+density so I don't have high hopes that you'll be able to solve it, you're always welcome to try though.

Makes sense. It might become a bit of a rabbit hole to rewrite that mixin, I'll close this PR and pick something else 👍

Yes, the last test ("Showcase") is a percy snapshot. I don't think it's publicly accessible though.

Good to know!

@floroz floroz closed this Jan 30, 2023
@floroz floroz deleted the feat/16288-vbtn-icon-size branch January 30, 2023 10:18
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.

2 participants