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: refacto CSS #500

Merged
merged 26 commits into from
Jun 10, 2024
Merged

feat: refacto CSS #500

merged 26 commits into from
Jun 10, 2024

Conversation

roushou
Copy link
Contributor

@roushou roushou commented Jun 9, 2024

Taking the opportunity, as the codebase is still relatively small, to refactor CSS styles with a proper use of TailwindCSS. The current usage of creating classes (also just for a single usage) with Tailwind classes quite defeats the purpose of Tailwind. And with the codebase growing up there might be some maintainability issues.

There's also a section in their documentation saying to be cautious about the use of the @apply directive https://tailwindcss.com/docs/reusing-styles#avoiding-premature-abstraction

TLDR:

  • Bigger CSS bundle
  • More mental overhead:
    • Losing co-location
    • Invent classnames (ock-textinput-input) -> Need to remember to remove them if not used anymore
    • Losing some LSP features

This PR basically places the content of classes defined in CSS files into their respective jsx className. Tailwind users might be more familiar with this way of doing imo.

The build script outputs CSS styles from auto-detected content so you don't have to bother maintaining the entrypoint index.css

I also added a utility for better classes merging.

FWIW The documentation is a bit off to me.

  • I don't think displaying the details of styles is relevant, I would be more interested in ways to modify them.
  • It's a pain to maintain, especially the content in return html tabs. There are a lot of copy/paste left and right.

I took some extra steps with the documentation but I think it could be improved separately.

@Zizzamia
Copy link
Contributor

Zizzamia commented Jun 9, 2024

I agree on so many points with you on this PR.

BUT, there is a trade-off I think it's stopping us to go full on Tailwind. Make it super easy to customize any component in OnchainKit.

For example, if I a component like

<potato />

which is just a one liner, I can simply force-replace the classname

<div className={{ className || 'rounded-full' }}  />

BUT, and here where there is my headache, if the same component has multiple levels

<div className={{ className || 'rounded-full' }}>
   <div className="border border-transparent" />
</div>

than the developers has no chance to customize the inside part of the component.

Also we don't want to force a company to use Tailwind just to have OnchainKit working. We want sites that don't use Tailwind to also use OnchainKit.

@roushou this is quite a balance we need to figure out. What do you think?

@roushou
Copy link
Contributor Author

roushou commented Jun 10, 2024

The current way of managing styles is not right to me too. It reads like:

"Hey here is how we did our styling, if you want to change something you have to write your own styles from scratch so checkout the documentation where you'll find our tailwind classes so make sure to learn tailwind too."

Speaking generally but I often don't want to spend time checking how things are under-the-hood when consuming a library. I'm more interested to know how to make it work the way I want with minimal efforts. So things like <div className={{ className || 'rounded-full flex ...' }} /> doesn't fit there because I would need to make sure to rewrite the whole styles correctly even if I just want to change the border.

At some point, things should be opinionated. A way I would recommend is to decompose components into subcomponents and expose them. Similar to what Radix UI does with its primitives. A huge advantage is that components can be composed however the user wants and styles of each subcomponent can be customized as needed.

For example, the Avatar component:

<Avatar.Root className="flex ...">
  <Avatar.Image
    className="w-36 h-36 ..."
    src="https://images.unsplash.com/photo-1492633423870-43d1cd2775eb?&w=128&h=128&dpr=2&q=80"
    alt="Colm Tuite"
  />
  <Avatar.Fallback className="border border-red-500 ..." delayMs={600}>
    CT
  </Avatar.Fallback>
</Avatar.Root>

Then under-the-hood classes need to be merged correctly with something like the little util cn I added. It uses tailwind-merge which prioritize the last conflicting classname. This way current styles can be easily changed using Tailwind with minimal effort.

If full customizability is needed like allowing to style without using Tailwind then taking inspirations from Radix again can be the way. In this case, by providing:

  • A headless component library
  • A component library with styled applied to the headless components.

Imo the fastest way would be to first go opinionated with components/subcomponents already styled and extensible with Tailwind. Then give more flexibility with a headless library when things are figured out.

@kyhyco
Copy link
Contributor

kyhyco commented Jun 10, 2024

@roushou you do bring up some valid points. Sharing context on what we have thinking so far.

Context

Here is our general approach so far:

  • Provide CSS class names to override
  • Provide CSS variables to override
  • Provide react utility functions to let folks build on their own

className prop override

So far, we have released primitives but about to release a few composite components. className prop override doesn't work well in composite components where we give you a component made up of smaller components. (More on this down below).

Initially, we decided not to release TokenSelectorDropdown and TokenSelectorModal and show only examples like in the TokenKit introduction page. With composite components, it's difficult give CSS controls to individual parts.

That is to say, for primitives, we hope to provide className prop override when it makes sense.

CSS class name and CSS variables override

Now with direct CSS override... we can provide either:

  • CSS class names to override styles individually
  • CSS variables to override styles systematically

While we use tailwind for our project, we don't want the end users to be forced to use tailwind to modify CSS styles. So we came up with the solution we have today despite tailwindcss not recommending generating class names and the reasons you stated what amounts to "poor abstraction" forcing users to read the source code.

On the tailwind side, we are in discussion whether to introduce ock prefix to tailwind so it won't interfere with existing tailwind projects if they modified the CSS primitives.

cn function is a great call out! It's a good one.

Utility functions and react hooks

I'd suspect that if default styles don't work well with existing user systems, users would benefit most from the utility functions, react hooks and data fetching functions and skip the whole UI layer. (That's what I would do).

shadcn + radix

Not fully explored but also in consideration where we wholesale import the whole kits into users code base so they can do whatever they want from there.

Radix approach is interesting. How to breakdown components and what to expose is a huge discussion topic right now given that we are about to release more composite components very very soon.

Note

Imo the fastest way would be to first go opinionated with components/subcomponents already styled and extensible with Tailwind.

We are totally aligned there. We are shipping out the core functionality first. We definitely understand our approach to extendability is not ideal at the moment. Huge part for us is to listen to community feedback like you have graciously providing in the last few weeks!

That all to say, loving all your thoughtful PRs and discussion topics. We will come back to this PR after a team discussion on this topic.

Cheers!

Copy link
Contributor

@ilikesymmetry ilikesymmetry left a comment

Choose a reason for hiding this comment

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

The tradeoff I see is between:

  1. Prioritize one CSS approach -> make it great for those who use it, but excludes others
  2. Prioritize CSS agnosticism -> support basically everyone, but make the median experience less than ideal

As always, pros and cons for both.

My personal opinion on building new things is that someone has to love it for it to succeed. If we build the best component library intentionally for Tailwind, everyone in web3 who uses Tailwind will love it. If we can reach some level of success and there is demand for other CSS approaches (eg I also see styled-components a fair amount), then we earn the right to provide another component library for them, akin to how wagmi started with React and now expanded support for Vue.

The worst case is no one loves us. If that happens, we expend a lot of effort for a tool no one wants to use because it's not producing enough value for the effort to integrate. Speaking from personal experience, I have always built Tailwind apps and I would never use a component library that asked me to change that. If I was using styled-components instead, I think it would be the same. I would much rather prefer consistency in my codebase than frankenstein something for short-term gains, but long-term pain.

I think we should move forward with an opinionated library that chooses one approach to CSS, ideally whatever is most used across the industry (especially new apps). I trust all of your intuition more than my own on what that would be, but I am happy with either Tailwind or styled components.

@kyhyco
Copy link
Contributor

kyhyco commented Jun 10, 2024

Created two PRs to discuss font and css variables separately. These PRs augments this PR's approach on removing CSS class names and directly using tailwind class names.

docs: use Inter font for doc site
feat: css variables

@kyhyco
Copy link
Contributor

kyhyco commented Jun 10, 2024

@roushou we decided to move away from the class name approach and directly use tailwind class names.

We will generate two separate css files.

  1. Includes unique OnchainKit styles
  2. Includes unique OnchaintKit styles + all tailwind primitives so folks without tailwind can still use OnchainKit

@@ -33,6 +33,10 @@
"viem": "^2",
"wagmi": "^2"
},
"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kyhyco this should probably be devDependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually should be a peer

@Zizzamia Zizzamia marked this pull request as ready for review June 10, 2024 23:50
@Zizzamia Zizzamia merged commit 4e37fc7 into coinbase:main Jun 10, 2024
5 of 6 checks passed
@roushou roushou deleted the refacto-css branch June 11, 2024 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants