-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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(VIconBtn): create new component #21114
base: dev
Are you sure you want to change the base?
Conversation
decbbe4
to
9af9cfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing variant prop?
}, 'VIconBtn') | ||
|
||
export const VIconBtn = genericComponent<VIconBtnSlots>()({ | ||
name: 'VIconBtn', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since our button-related components have names such as VBtnToggle
, VBtnGroup
, it would make more sense to name this component VBtnIcon
rather than VIconBtn
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does everyone think? I chose the name for 2 reasons:
- VCheckboxBtn
- That's what Google calls it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with VIconBtn
. I think the last word should be what it is, a btn.
Sample size of 1 but looks like 1000x v-btn = 380ms and 1000x v-icon-btn = 270ms (29% faster) |
{ | ||
'--v-icon-btn-rotate': convertToUnit(props.rotate, 'deg'), | ||
'--v-icon-btn-height': props.size ? convertToUnit(props.size) : undefined, | ||
'--v-icon-btn-width': props.size ? convertToUnit(props.size) : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No named sizes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's meant to be more intentional in it's size, it's why I didn't add density either.
removes overlay that is present on focus and active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good! I don't think a density prop is needed since icon will remain the same size and btn size can be adjusted via size
prop.
Motivation and Context
TODO: https://m3.material.io/components/icon-buttons
Markup: