-
Notifications
You must be signed in to change notification settings - Fork 217
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
Create a new VIconButton
#2697
Create a new VIconButton
#2697
Conversation
e258c38
to
ee7863b
Compare
Size Change: +1.4 kB (0%) Total Size: 839 kB
ℹ️ View Unchanged
|
Full-stack documentation: https://docs.openverse.org/_preview/2697 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
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.
LGTM! It seems like we're losing the tiny
and extra-large
sizes, is that intentional?
@@ -33,17 +38,20 @@ export const Template = (args) => ({ | |||
export const sizesTemplate = (args) => ({ | |||
template: ` | |||
<div class="flex gap-x-2"> | |||
<div v-for="size in ['tiny', 'small', 'medium', 'large', 'extra-large']" :key="size" class="flex flex-col items-center p-2"> | |||
<div v-for="size in args.baseButtonSizes" :key="size" class="flex flex-col items-center p-2"> |
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.
Nice!
@obulat could you clarify the intention of this PR? It creates the new icon button, but doesn't actually implement it anywhere, right? Does that mean that the button styles do need to change in the app (which will require updating visual regression tests) and that we will do that in subsequent PR(s)? I'm mostly curious about the approach taken and if it would make more sense to create the new VIconButton but backwards-compatible, with the old sizes included (to be deprecated later), rather than maintain two versions for a (likely brief) period. |
b6c8276
to
bca558e
Compare
Yes! Those sizes were previously used for Audio controls. Now the designs have updated buttons with fewer sizes - that's why we are updating. |
bca558e
to
d2d5df3
Compare
d2d5df3
to
44328b2
Compare
The
The change in "close" buttons will cause changes in the visual regression snapshots.
Current component ( |
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 looks great ✨ 🚀
I noticed two prop variants I wasn't aware of, but do not block the work made: filled-transparent
and transparent-tx
. The first one is also an oxymoron 😅
Fixes
Related to #460 by @fcoveram
Description
This PR adds a new
VIconButton
component using the updated styles in the Figma mockupsThe old styles for
VIconButton
were used mainly in the popover/modal close buttons, audio controls and the buttons within the search bar. They will gradually be updated to use the newVIconButton
.Testing Instructions
Check out the new
VIconButton
story. It should have all of the styles from the Figma mockup. Old buttons (listed above) should also look the same.Check the rendered Story: https://docs.openverse.org/_preview/2697/storybook/?path=/docs/components-viconbutton--default-story
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin