-
Notifications
You must be signed in to change notification settings - Fork 0
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
Tailwind #71
Open
corbanbrook
wants to merge
20
commits into
master
Choose a base branch
from
tailwind
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Tailwind #71
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This aims to replace the Vanilla Extract Atomic classes (Sprinkles) with Tailwind.
TODO
What does this mean?
First of all the
<Box>
component which adds atomic classes props ie:Will be converted to tailwind class names
Migrating
This wont be that much work to refactor on design-system side but will require thousands of edits across projects that utilize the design-system like wallet-webapp, builder, sequence kit, demo-dapp, and status-page.
To avoid that tedious manual work, this PR includes a codemod via facebook's jscodeshift tool.
The codemod is a work in progress but can already handle the majority of cases, converting atomic class props with
Literal
value types to their respective tailwind className ie, paddingX="4" -> className="px-4".(Luckily when designing the design-system we based many of the prop values off of the names used in tailwind like their 4px spacing system so it is easy to convert)
Work will continue to improve the codemod so it can transform
JSXExpressionContainer
withConditionalExpression
,CallExpression
, andTemplateLiteral
types.To run:
pnpm codemod <path to tsx files>
pnpm codemod ../wallet-webapp/src/**/*.tsx
Some Benefits
Will no longer need to merge JSXIntrinsic props and Atom props means typescript needs to do less work and no more worry of conflicting props between the two.
Typescript type checking performance: related to the issue above, typescript gets very slow when adding so many atomic class props to each Box element. and since everything inherits from Box it pushes it to the limit. Tailwind on the otherhand is just string literals added to a single
className
prop so no work to do. VSCode and others have plugins for tailwind support to add intellisense support which is faster than typechecking.Vanilla extract needs to bundle a mapping of all atomic properties (with values and conditionals) to their hashed css classnames. ie:
A mapping needs to be created for each property by value by condition (breakpoints, selector states: hover, disabled, etc) so this can get big quickly.
So it still needs to include a bunch of javascript even tho its not technically considered CSS-in-JS. Tailwind does not have this issue because the classnames are human readable and known ie,
px-4
it only ships plain css which should result in smaller overall bundle sizeThe current mapping represents 110KB of the 670KB js bundle (16%), Gzipped: 20KB of 135KB (15%).
display="flex" flexDirection="column" marginX="4" paddingY="2" gap="1"
vs
className="flex flex-col mx-4 py-2 gap-1"
This means bundle sizes should be slightly smaller but more importantly it is much quicker to type and see more on screen.
tailwind has better support for merging classes and order precedence with tailwind-merge. This makes classes behave similar to css rules, last rule wins.
Vanilla extract has no support for purging unused atomic classes. So it ships all classes for all values + conditions even if you don't use them. Tailwind on the other hand has great support for this and you can even detect and merge and purge unused across multiple projects, for instance design-system + wallet-webapp. This means we will ship the smallest possible css.
Niftyswap team has already switched over to tailwind and they are loving it. would be good to standardize on the sequence side as well.
No longer need a Polymorphic Box Component: Polymorphism in React, eg:
<Box as={Link} to="/wallet" marginTop="3" />
is the death to typescript performance. We currently have a single Box component that includes the atomic class props and everything inherits from it it works well and type checks well, but slow. In most cases we wont need this anymore because Tailwind is just classes globally available to any component in the project so i can simply use<Link className="mt-3" to="/wallet" />
.Typescript 5 and Polymorphic Components
In addition, supporting polymorphic components means we are stuck on typescript 4. Typescript 5+ made changes that removed support for the way we are able to type refs, callbacks and spread props when using
as={Component}
. As we had to jump through some hoops in typescript 4 to even get this to kind of work and incur the associated performance costs i dont think it is something they ever plan to support.