-
Notifications
You must be signed in to change notification settings - Fork 2
EGG-44 Handling padding calculating when border and padding exist together (for inner stroke only) #69
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
@@ -285,7 +285,7 @@ const tailwindClassGenerators: Record<string, Generator> = { | |||
opacity: (token) => generateTailwindOpacityClass(token), | |||
}; | |||
|
|||
export function createTailwindClasses(tokens: NonNullableStyleToken[]): string[] { | |||
export function createTailwindClasses(tokens: GeneratorToken[]): string[] { |
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.
GeneratorToken
is a subset of NonNullableStyleToken
. Generators don't require all the properties that NonNullableStyleToken
has, this will allow for easier refactor later.
|
||
const cssByVariantCombinations = generateCombinatorialStyles(mixins); | ||
return Object.entries(cssByVariantCombinations).map(([, cssByVariantCombination]) => { | ||
const path = cssByVariantCombination.path; |
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.
this original was a bug where all of the components within this component set shared the same path (implying they all had the same variant property and values)
Updated the flow to include path up until this point to correct this
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.
There's a unit test to verify that this behavior stays this way
@@ -1,6 +1,6 @@ | |||
export interface PathNode { | |||
name: string; | |||
type: SceneNode['type']; | |||
type: (BaseNode | (BaseNode & ChildrenMixin))['type']; |
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.
this is the actual type. We use SceneNode
a lot in the code base through type assertion. We should find a type safe way to confirm this instead of casting.
unit: true, | ||
}; | ||
|
||
const rounded = (value: number, precision: number): number => { |
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.
rounding when converting from px to rem causes styling issues where things are off slightly:
1px -> 0.62rem is actually 0.992px and not 1px
without rounding we get the proper conversion:
1px -> 0.625rem (which is 1px)
@@ -66,11 +66,11 @@ $spacing-9: 0.48; | |||
background: #ffffff; | |||
opacity: 0.5; | |||
} | |||
@mixin opacity-custom-group_opacity-custom { | |||
@mixin opacity-custom-group-opacity-custom { |
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.
selectors using the default config (css and scss) only use _
for separating variant property and value. Shouldn't be used unless both are present.
node
a required argument instead of an optional one. Simplified all usesNote: The snapshots doubled in size due to testing both template and combinatorial. So there's a lot of new lines added, but the core changes are still verifiable if you look pass all the added lines and just the changed lines