-
Notifications
You must be signed in to change notification settings - Fork 152
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
Remove Styled Components #4317
Remove Styled Components #4317
Conversation
Deploying orbit with Cloudflare Pages
|
12f029e
to
0b2a0cb
Compare
Storybook staging is available at https://kiwicom-orbit-tw-refactor-orbit-provider.surge.sh |
Size Change: -4.67 kB (-1%) Total Size: 436 kB
ℹ️ View Unchanged
|
0b2a0cb
to
62083ef
Compare
94f87d2
to
ccbc8db
Compare
<Container key={icon}> | ||
<div | ||
className="gap-lg bg-white-normal mb-lg rounded-large border-cloud-normal px-lg flex w-full flex-row content-center items-center justify-start border border-solid" | ||
style={{ minHeight: "80px" }} |
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.
Why not min-h-[80px]
?
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.
I wanted to avoid that custom class to be built in the final CSS file. Since this component is to be used only in Storybook, I prefer the style to be inline, so that class is not unnecessarily created by the Tailwind compiler
style={{ | ||
fontFamily: | ||
'"SFMono-Regular", Consolas, "Liberation Mono", Menlo, Courier, monospace', | ||
fontSize: "12px", |
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 could also be a class, though I agree that the font family is probably easier like that?
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.
Yes, for font-family it was just for convenience. And for font-size it's the same reason as above
|
||
## Utilities | ||
|
||
`styled-components` doesn't know automatically how to switch `margin`, `padding` and i.e. `left` properties to the other direction. So it's necessary to use some `utilities` inside your styles. |
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.
How does Tailwind do it? Does it rely on logical properties?
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.
Yes. Tailwind has builtin classes that handle it, eg: ms-
will apply margin-inline-start
, that browsers translate to left margin in LTR and right margin in RTL.
One can also use the rtl:
prefix in classes to apply styles that are only applicable in RTL
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.
Would it make sense to include this info in the migration guide, just to point people somewhere? I'm sure I wouldn't be the only one asking that question :)
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.
We can, I just wanted to make the migration guide a bit more technology-agnostic. Because if one is still using styled-components those tailwind tips won't be that helpful, I'd say. Ultimately, it is up to each project to decide how they will handle RTL and Media Queries, based on the styling solution they are currently using (even considering the open-source side of Orbit).
I am ok with adding Tailwind example as a proposed solution for those using that technology, though. We can also link to the Tailwind docs regarding those topics.
|
||
### rtl utils removed | ||
|
||
The `rtl` utility functions have been removed. They were used to handle RTL styles in the `styled-components` version of Orbit. |
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.
Maybe add a comment as to how it works with Tailwind/why it's okay for this to be removed.
Also, if folks were relying on them, would it make sense to provide a code snippet replacement?
(same question for mediaQueries
above actually?)
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.
I did some research and did not find relevant usages for those, hence why I didn't put that much effort into writing alternatives.
I did not write anything about how to achieve results in Tailwind because teams that are not yet in Tailwind and might need something will have to decide on how to handle these (I don't know if there is any, tbh).
I can add a few examples in the style of before and after scenarios, but I am afraid they will either be too incomplete (I believe it's too much to cover all scenarios) or too specific, so I am not entirely sure about what to add.
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.
Okay, got it.
Maybe it still make sense to just point out to Tailwind docs for RTL and media queries
Maybe we could still offer a permalink to the previous RTL utility functions for folks to copy them over in their codebases in case they'd need them?
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.
I think I answered part of this in the comment above.
Regarding the link to old code, I personally don't like it that much that we have links to dead code, but I can add it, for sure.
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.
You know, it's just to ease adoption: if people want to update but they have a bunch of code relying on these removed features, they can drop them in their codebase temporarily and then do a proper refactor later on.
I personally don't want to encourage it, but we want people to use the latest version of Orbit.
fbcdda8
to
ddc783e
Compare
7c7e9ad
to
feceb02
Compare
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.
Minor nitpicks 💅
### mediaQueries util function removed | ||
|
||
The `mediaQueries` function is no longer available. This was a styled-components util function that is now no longer expected to be used, as we move away from StyledComponents. | ||
If needed, the tokens and breakpoint values are still accessible via the `getBreakpointWidth` function and the `QUERIES` and `TOKEN` constants. | ||
Check [the documentation](/development/utilities/media-queries/) for more information on how to use these. | ||
|
||
If you are still using styled-components in your custom components and you need this function we invite you to add the implementation to your project. The implementation can be seen [here](https://github.com/kiwicom/orbit/blob/%40kiwicom/orbit-components%4014.0.0/packages/orbit-components/src/utils/mediaQuery/index.ts#L23). |
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.
For consistency.
If you are still using styled-components in your custom components and you need this function we invite you to add the implementation to your project. The implementation can be seen [here](https://github.com/kiwicom/orbit/blob/%40kiwicom/orbit-components%4014.0.0/packages/orbit-components/src/utils/mediaQuery/index.ts#L23). | |
If you are still using `styled-components` in your custom components and you need this function, we invite you to add the implementation to your project. The implementation can be seen [here](https://github.com/kiwicom/orbit/blob/%40kiwicom/orbit-components%4014.0.0/packages/orbit-components/src/utils/mediaQuery/index.ts#L23). |
</ThemeProvider> | ||
); | ||
``` | ||
|
||
### mediaQueries util function removed | ||
|
||
The `mediaQueries` function is no longer available. This was a styled-components util function that is now no longer expected to be used, as we move away from StyledComponents. |
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.
The `mediaQueries` function is no longer available. This was a styled-components util function that is now no longer expected to be used, as we move away from StyledComponents. | |
The `mediaQueries` function is no longer available. This was a `styled-components` util function that is now no longer expected to be used, as we move away from StyledComponents. |
You can add a new Component to storybook by adding the code below. | ||
All components should have stories that display their different variations and behaviors. | ||
Stories should be written in the file `Component.stories.tsx`. | ||
For more informations about using Storybook check the [official documentation](https://storybook.js.org/basics/guide-react/). |
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.
For more informations about using Storybook check the [official documentation](https://storybook.js.org/basics/guide-react/). | |
For more information about using Storybook check the [official documentation](https://storybook.js.org/basics/guide-react/). |
|
||
At the very least, it’s necessary to have the `Default` and `Playground` chapters defined. The `Default` chapter contains a Component without any additional props. The `Playground` chapter contains a Component with the possibility to change all defined props for full customization. Follow the structure of the code below. For more information check the Chapter add-on doc. | ||
Visual tests are written in the file `Component.ct.tsx` and should be used to test the component in different states and with different props. |
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.
As we mentioned above we use Jest for unit testing, what do you think about mentioning Playwright here?
return <StyledComponent size={size}>...</StyledComponent>; | ||
}; | ||
``` | ||
We use `tailwind` for styling. Custom tailwind classes can be used but should be avoided. All design tokens from `orbit-design-tokens` have classes that apply their value to the different possible CSS properties. |
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.
We use `tailwind` for styling. Custom tailwind classes can be used but should be avoided. All design tokens from `orbit-design-tokens` have classes that apply their value to the different possible CSS properties. | |
We use `tailwind` for styling. Custom Tailwind classes can be used but should be avoided. All design tokens from `orbit-design-tokens` have classes that apply their value to the different possible CSS properties. |
We use tailwind, tailwind
, Tailwind, and Tailwind CSS indistinctly across all docs. It would be nice to be consistent on a unique form.
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.
I think it's ok to use Tailwind
(or Tailwind
, but consistently) as a short for TailwindCSS
. For this particular sentence, I rephrased it to:
We use TailwindCSS for styling. Custom Tailwind classes can be used but should be avoided.
What do you think?
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 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.
Yeah, we're using both forms the official version (with space) across all docs 🤦♂️ I'll unify on a separate commit fix for this one sentence
feceb02
to
45c7478
Compare
ddc783e
to
2aecfa8
Compare
ceae4a3
to
9c9aca6
Compare
2aecfa8
to
aab97f3
Compare
9c9aca6
to
81486b9
Compare
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.
Internal component no longer used. Leftover from the TW migration
Internal non-exported component that is no longer used with Tailwind. The consts were kept and moved to other locations. The docs were also updated.
Internal unused hook. This was not being exported and relied on Styled Components
Internal component used only for Storybook docs.
Text and TextLink were deprecated waiting for other components to be migrated to TW. They are no longer used anywhere so they are being removed.
Some internal utils were no longer used and relied on styled-components.
RTL utils removed. They were styled-components utils. Some functions are kept since they're still used internally. BREAKING CHANGE: rtl styled-components utils removed.
Old transforms used styled-components for testing. They can be removed now.
OrbitProvider no longer uses Styled-components' ThemeProvider. Instead, we are using a custom ThemeProvider. The useTheme hook was also updated accordingly. BREAKING CHANGE: Styled-Components' ThemeProvider is no longer used in OrbitProvider. If you still need styled-components capabilities, you should add the ThemeProvider from styled-components to your app and pass it the same theme.
Some reword and removal of styled-components mentions
81486b9
to
fd70526
Compare
Created on top of #4313.
Remove Styled Components from the
orbit-components
package.The OrbitProvider no longer includes the styled-components' ThemeProvider. This should have no impact for teams not using styled-components.
Docs and migration guide updated accordingly.
FEPLT-1923