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

Core upgrade xstyled styled components v6 #2436

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

P1X3L
Copy link
Contributor

@P1X3L P1X3L commented Mar 27, 2024

  • use xstyled fork with styled-components v6.1.8
  • upgrade styled-components peer dep to v6.1.8

Copy link

👀 Visit Preview

@github-actions github-actions bot added the wip label Mar 27, 2024
@P1X3L P1X3L force-pushed the core-upgrade-xstyled-styled-components-v6 branch from 6ff3875 to 4d86cd8 Compare March 27, 2024 14:44
@@ -19,7 +19,7 @@ Here you'll find all the core components you need to create a delightful webapp.
1 - Install the **peer dependencies** listed below:

```bash
yarn add @xstyled/styled-components react styled-components
yarn add @wttj/xstyled-styled-components react styled-components
Copy link
Contributor

Choose a reason for hiding this comment

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

is it public ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet but it's will be discuss tomorrow :)

Comment on lines 60 to 62
*/
export const system = compose<WuiSystemProps>(...SYSTEM_PROPS)
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

since we've stopped using it in favor of *Box components; should we remove it from the exports ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need it to be able to override style props in particular use cases

Comment on lines +63 to +66
${lines &&
lines !== Infinity &&
Object.keys(lineHeightHeadingsFixer).includes(variant) &&
fixLineHeightStyles(variant)};
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont understand how this is related to the upgrade ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has nothing to do with the upgrade indeed 🙈
Just couldn't keep up with those g, y, q, j and p being cut

Comment on lines +3 to +18
export const getMax = (
width: SystemProp<string | number, Theme>,
height: SystemProp<string | number, Theme> = width
): SystemProp<string | number, Theme> => {
const typeToValueExtractor: Record<
string,
(value: SystemProp<string | number, Theme>) => number
> = {
string: (value: string) => parseInt(value, 10),
number: (value: number): number => value,
}
const widthNumber =
typeToValueExtractor[typeof width as keyof typeof typeToValueExtractor]?.(width) || 0
const heightNumber =
typeToValueExtractor[typeof width as keyof typeof typeToValueExtractor]?.(height) || 0
const diff = widthNumber - heightNumber
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont see the problem that has been solved here, could you clarify it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the width andheight arguments were typed as string but have been enriched after the upgrade of xstyled & styled-components resulting in type errors

@P1X3L P1X3L force-pushed the core-upgrade-xstyled-styled-components-v6 branch from b81e57c to 1c313e7 Compare April 4, 2024 16:03
@P1X3L P1X3L force-pushed the core-upgrade-xstyled-styled-components-v6 branch from 1c313e7 to ab2bbbc Compare April 4, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants