This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Audit VIconButton sizes and props #1928
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1928 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. |
obulat
changed the title
openverse-bot
added
✨ goal: improvement
7 tasks
|
Size Change: -3.4 kB (0%) Total Size: 811 kB
ℹ️ View Unchanged
|
Replace all `size`s with `w-x h-x` classes Reuse VSearchBarButton instead of a separate VClearButton component Add 8 to safelist and fix iconSize (it's string, not a number) Revert changes to plain--avoid button Add size classes to all VIcons Add size classes to all VIcons
obulat
force-pushed
the
re-add_size_to_vicon
branch
from
6bcef8d to
3da22e9
Compare
dhruvkb
approved these changes
Oct 25, 2022
obulat
force-pushed
the
re-add_size_to_vicon
branch
from
3b451f7 to
8b08bf7
Compare
I did forget to add the tests! I went back to add some tests, and also re-audited all the VIconButtons to make sure that we have consistent sizes (and removed all of the unused ones). Could you have one more look at this PR? |
obulat
changed the title
sarayourfriend
approved these changes
Oct 27, 2022
There was a problem hiding this comment.
Love the tests and really appreciate how much confidence the visual regression tests give us when working on these things. Kudos to everyone who's been writing more and more of them 🚀
The changes LGTM. I left just one non-blocking note about the names of the sizes.
Comment on lines
32
to
33
| larger: { icon: 8, button: 'w-14 h-14' }, | ||
| large: { icon: 12, button: 'w-20 h-20' }, |
There was a problem hiding this comment.
Just a nit pick on the names here: In my head I expected larger to be bigger than large 🤔 Maybe larger -> large and large -> extra-large or something would be clearer in the "order" of how big one or the other is?
There was a problem hiding this comment.
Thank you for helping with the naming :)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #1894 by @obulat
Description
This PR is an alternative to #1895. It keeps using the
sizeprop because otherwise, we would have to use classes like!w-4to override the default icon size, or not have a default icon size and add size classes to every VIcon..This PR audits all the
VIconButtons:There is a Storybook VR test for
VIconButtonsizes.button-propsdeclarations are re-written. There's no need to passaria-labelorvariant="plain"tobutton-propsbecause the first can be passed directly to theVIconButton, and the second is the default.This PR also adds a Storybook story for VSearchBarButton; replaces
VClearButtonwith aVSearchBarButtonin the desktop search bar.Testing Instructions
The CI should pass, including all the audio VR tests that use various sizes of
VIconButton, and all of the Storybook tests.Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin