Skip to content
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

feat: Add box-sizing:border-box automatically to all styles created by createStyles #2721

Conversation

RayRedGoose
Copy link
Contributor

@RayRedGoose RayRedGoose commented May 2, 2024

Summary

Resolves: #2720

This PR adds box-sizing:border-box to all style created by createStyles as we do the same for stencils. Also, createModifiers has been updated to support styles passed as an object to remove usage of createStyles.

Release Category

Infrastructure

BREAKING CHANGES

Adds box-sizing: border-box to all styles created by createStyles. If you did not add this style already, it may change the way width works for the component. Our intent is to make all elements use border-box layouts to make width calculations more predictable. This change may change the way your component works if you use the width style property.

Thank You Gif (optional)

Car Fixing GIF By The Back Roads

@RayRedGoose RayRedGoose added the ready for review Code is ready for review label May 2, 2024
Copy link

cypress bot commented May 2, 2024

Passing run #7265 ↗︎

0 970 3 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 6e65036 into 9caa780...
Project: canvas-kit Commit: 7079b17d1c ℹ️
Status: Passed Duration: 17:10 💡
Started: May 2, 2024 8:23 PM Ended: May 2, 2024 8:40 PM

Review all test suite changes for PR #2721 ↗︎

@@ -55,7 +54,7 @@ export const InformationHighlight = createContainer('section')({
return (
<Base
as={Element}
cs={[containerStyles, containerModifiers({variant: model.state.variant})]}
{...informationHighlightStencil({variant: model.state.variant})}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While a stencil does return a spreadable object, this pattern doesn't handle merging. There's a very simple way to break InformationHighlight:

<InformationHighlight className="whoops" />

The className prop returned by the stencil will be overwritten by the incoming prop. At this point, Base isn't doing anything useful for us.

This is also why I'm not a fan of us using a Base component because even though we can use the cs prop, our component cannot handle the cs prop because it will get overridden instead of merged. I think the safest way these components work is by using handleCsProp and dropping Base:

return (
  <Element {...handleCsProp(props, informationHighlightStencil({variant: model.state.variant}))} />

@@ -421,13 +423,21 @@ export function createModifiers<M extends ModifierConfig>(input: M): ModifierRet
const modifierFn = (modifiers: Partial<ModifierValues<M>>) => {
return Object.keys(modifiers)
.filter(key => input[key] && (input as any)[key][modifiers[key]])
.map(key => (input as any)[key][modifiers[key]])
.map(key => (modifierFn as any)[key][modifiers[key]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will conflict with #2719

@NicholasBoll
Copy link
Member

There's something that's missing here: A createModifiers style transform. Originally there wasn't one, because createModifiers didn't call createStyles implicitly. But now you'll find the style transform no longer processes createModifiers.

Current

// source
const myModifiers = createModifiers({
  size: {
    large: createStyles({padding: 20}),
    small: createStyles({padding: 10})
  }
})

// transform
const myModifiers = createModifiers({
  size: {
    large: createStyles({name: 'abcde1', styles: 'padding:20px;'}),
    small: createStyles({name: 'abcde2', styles: 'padding:10px;'}),
  }
})

With this change

// source
const myModifiers = createModifiers({
  size: {
    large: {padding: 20},
    small: {padding: 10}
  }
})

// transform
const myModifiers = createModifiers({
  size: {
    large: {padding: 20}, // should be {name: 'abcde1', styles: 'padding:20px;'} like stencils
    small: {padding: 10}
  }
})

This change means createModifiers lost transformation.

@alanbsmith
Copy link
Member

This is a bit more involved than we initially thought. We're going to close it for now, and we can revisit later.

@alanbsmith alanbsmith closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants