-
Notifications
You must be signed in to change notification settings - Fork 30
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
Change vertical gap to 4px in beta containers #13126
base: main
Are you sure you want to change the base?
Conversation
Size Change: +13 B (0%) Total Size: 871 kB ℹ️ View Unchanged
|
@@ -128,16 +129,16 @@ const decidePosition = ( | |||
}; | |||
|
|||
/** Detemines the gap size between components in card layout */ | |||
const decideGap = (gapSize: GapSize) => { | |||
const decideGap = (gapSize: GapSize, isBetaContainer: boolean) => { |
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.
Instead of updating the sizing here, could we update the getGapSize function to pass through tiny
as the gap size prop (which is 4px)?
d7acc43
to
2986579
Compare
2986579
to
c0ec4c5
Compare
b381d69
to
1e17403
Compare
1e17403
to
8a74765
Compare
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
8a74765
to
90ee12c
Compare
What does this change?
For cards with a vertical layout and belonging to a beta container, reduce the gap between the image and the meta (i.e. comment count) to tiny (4px).
This PR introduces a new condition to the
getGapSize
method, but the condition is low down in the priority order.This means there will be some exceptions to this condition, e.g. a 'scrollable small' container is a beta container, but prefers a medium size gap. This rule is preferred to the vertical gap rule.
I am unsure if this priority order is correct - would be good to have another pair of eyes.
Why?
Design spec.
Screenshots