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

RFC: styling the UI library #1406

Merged
merged 2 commits into from
Jul 10, 2024
Merged

RFC: styling the UI library #1406

merged 2 commits into from
Jul 10, 2024

Conversation

jessepinho
Copy link
Contributor

@jessepinho jessepinho commented Jul 3, 2024

This PR is a request for comments on the ADR I've written. Please read the ADR (Markdown preview here), and make inline comments on the file or general comments here in the PR thread.

I've written the ADR as though it's a finished decision, but obviously it's not — I just wrote what my decision would be, as a starting point for the rest of the team to comment on it.

Since our web weekly tomorrow is canceled for July 4th, we'll need to talk about this issue asynchronously here in the PR. Perhaps we can come to a final decision at next Thursday's web weekly, if we haven't already resolved the issue here.

Related to prax-wallet/prax#81

Copy link

changeset-bot bot commented Jul 3, 2024

⚠️ No Changeset found

Latest commit: 3265a81

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jessepinho jessepinho force-pushed the jessepinho/ui-library-adr branch 2 times, most recently from f6bcd47 to 04319e9 Compare July 3, 2024 18:44
@jessepinho jessepinho changed the title WIP: UI library styling ADR RFC: styling the UI library Jul 3, 2024
@jessepinho jessepinho marked this pull request as ready for review July 3, 2024 18:54
@jessepinho jessepinho requested a review from a team July 3, 2024 18:54
@grod220
Copy link
Collaborator

grod220 commented Jul 5, 2024

It's really too bad there isn't some build tool that allows non-tailwind projects to use tailwind components. Given that's the case, I think the reasoning here makes sense.

The question then becomes, which css-in-js framework/method should we use? Ideas:

@VanishMax
Copy link
Contributor

@grod220 @jessepinho Tailwind can be built just like any other solutions – by extracting CSS from JS, and consumers won't need to know about Tailwind existence. Take a look at the vite-react-component-library-starter template, it builds into index.js, index.d.ts, and index.css. Theoretically, we could reconfigure it to build different components separately (not into one index.js) and have CSS being exported in chunks to reduce the styling load

@jessepinho
Copy link
Contributor Author

@VanishMax we'd still run into the issues I've outlined here — particularly the last one, where we'd either have to duplicate CSS classes across component CSS files, or we'd have to export a massive kitchen-sink CSS with a bunch of code that consumers don't need.

@VanishMax
Copy link
Contributor

@jessepinho CSS chunks might solve the issue if configured correctly. Referencing your example, if three different Penumbra UI library components use bg-card-radial, then a chunk abc1123xyz.chunk.css file would be created and referenced by 1.css, 2.css, etc.

This is still a theory since I've done the same with JS chunks but not CSS but might be considered

@jessepinho
Copy link
Contributor Author

@VanishMax good point — if we could figure that out, then perhaps consumers' CSS compilers could compile all the resultant CSS chunks into a single CSS file, to prevent browsers from having to download a ton of tiny, single-class CSS files. That said, that might force us to either A) create a ton of tiny single-class CSS files to avoid making CSS compilers compile more than they need to, or B) create utility CSS files (for things like bg-card-radial alongside a bunch of other utility classes) which, again, would result in unused CSS getting compiled together. The tradeoff might not be huge though.

To me, the biggest issue is that we would be requiring consumers to import CSS files in addition to component JS files, which — while not uncommon — isn't a great DX if it can be avoided. I've added a note about this here: 3265a81

@jessepinho
Copy link
Contributor Author

@grod220 I could make a separate ADR/RFC for which framework to use, but I was personally leaning toward styled-components. It's the most popular one, and I've used it extensively in the past. However, emotion looks equally suited to the task.

Notes on the others:

  • styled-jsx requires a Babel plugin. I'm a little unclear on whether that would be required for consumers as well, but I suspect it would, since it appears to modify the base definition of the <style /> JSX component.
  • linaria and vanilla-extract (clever name!) both compile CSS files, so they would still require consumers to import CSS files alongside our component JS files.
  • Plain style* objects would give us the flexibility we need, but that basically means we're doing CSS-in-JS anyway, so we may as well use one of the established libraries like emotion or styled-components, IMO.

* You wrote "Plain objects passed to className prop" — I assume you meant the style prop?

Copy link
Collaborator

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

I believe styled-components had some issues with the new server components? Not sure, but we should ensure there's compatibility there with whatever we go with. It's a 👍 from me, but @VanishMax any further thoughts on this?

@jessepinho
Copy link
Contributor Author

@grod220 hm, I'm not familiar with those issues, but I'm seeing support in Remix and NextJS. Theoretically, we should probably discourage Penumbra UI consumers from doing server-side work anyway for privacy reasons, but 🤷🏻‍♂️

@VanishMax
Copy link
Contributor

One important aspect I forgot to mention previously about the UI kit is the accessibility. Using Radix-ui helps us maintain quality across UI components in terms of the full keyboard support but we should not forget about the color contrast, HTML semantics and screen reader accessibility.

Saying that, I propose we keep working with radix and possibly use some components from react-aria (e.g. Calendar or DatePicker, which is not supported in Radix).

Anyway, it doesn't relate to this RFC, just a thought. An approve from my side:)

@jessepinho
Copy link
Contributor Author

Excellent call-out, @VanishMax — I definitely will be leaning on existing libraries more complex components like date pickers, calendars, etc. For simpler components, I'll likely roll them myself to avoid messy base style overrides, or I'll use headless UI libraries (like Radix, as you suggested) when needed.

@jessepinho jessepinho merged commit acc5061 into main Jul 10, 2024
6 checks passed
@jessepinho jessepinho deleted the jessepinho/ui-library-adr branch July 10, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants