This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Refactor VIcon to set the size using a tailwind class instead of the size prop
#1895
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/1895 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. |
|
Size Change: -2.27 kB (0%) Total Size: 821 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
add/mobile_header_buttons
branch
2 times, most recently
from
66cc5af
to
fe8c412
Compare
obulat
added
🟧 priority: high
dhruvkb
reviewed
Oct 13, 2022
src/components/VButton.vue
Outdated
Comment on lines
13
to
14
| ? 'border border-tx' | ||
| : 'border border-tx focus-visible:ring focus-visible:ring-pink', |
There was a problem hiding this comment.
'plain--avoid' is used in several places. Changing the base style could have unforeseen changes that are not captured in VRT. Can you add snapshots to other usages of this button variant?
src/components/VIcon/VIcon.vue
Outdated
| * | ||
| * @default 6 | ||
| */ | ||
| size: { | ||
| type: Number, | ||
| default: 6, | ||
| validator: (val: number) => [4, 5, 6].includes(val), | ||
| validator: (val: number) => [4, 5, 6, 8, 12].includes(val), |
There was a problem hiding this comment.
Instead of this being a prop, the parent should be able to set h-* and w-* styles on the component. This seems a very contrived way to provide more sizes.
There was a problem hiding this comment.
Also the new sizes added here, being dynamic, should be added to tailwind.safelist.txt.
There was a problem hiding this comment.
Do you suggest that we scrap the size prop altogether, and just use h-* w-* classes set by the parent component, with a fallback of h-6 w-6?
There was a problem hiding this comment.
I'm suggesting scrapping the prop in the long term and adding the safelist to approve the PR.
There was a problem hiding this comment.
I do like the API design of size type props, but I think it makes the most sense when standardized across a component library with lots of small, composable ui building blocks. I agree with @dhruvkb's suggestion here.
There was a problem hiding this comment.
I've removed the prop, and added a dynamically generated size to the safelist.
There was a problem hiding this comment.
@zackkrida, @dhruvkb, I've tried using classes, but then there is a problem with default and overrides:
- if we add
w-6 h-6by default, then to override them, we would need to add!important modifier. - if we do not add a default, then we would need to add the size classes to all VIcons.
So, I went back to the size prop as a cleaner option, and opened an alternative PR: #1928
obulat
force-pushed
the
add/mobile_header_buttons
branch
4 times, most recently
from
99c72be
to
932d4e3
Compare
obulat
changed the title
VIconButton and add VSearchBarButtonVIcon to set the size using a tailwind class instead of the size prop
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
obulat
force-pushed
the
add/mobile_header_buttons
branch
from
24fccd4
to
5ee0c1c
Compare
7 tasks
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was updated 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes |
|
Closing this PR because we have decided that keeping the |
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 removes the numeric
sizeprop fromVIconcomponent. This is because there are too many sizes in the app to make it useful, especially since we have to add all the dynamically generated size classes (likew-${size} h-$size) to tailwind's safelist.Testing Instructions
You can view the buttons in the Storybook.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin