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

prax/ui #45

Closed
wants to merge 2 commits into from
Closed

prax/ui #45

wants to merge 2 commits into from

Conversation

TalDerei
Copy link
Contributor

No description provided.

Copy link

changeset-bot bot commented Jun 20, 2024

⚠️ No Changeset found

Latest commit: 31db831

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.

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

@TalDerei TalDerei marked this pull request as draft June 20, 2024 07:12
'./shared/**/*.{ts,tsx}',
'./node_modules/@penumbra-zone/ui/**/*.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

This line makes UI package work correctly with Tailwind.

Why: Tailwind performs JIT compilation to compose and export to a CSS file all used classes. In case of UI package, we have many dynamic classes, which are not parsed without this line change. For example, in <Button variant='gradient'> the variant dynamically adds classes and, to process them by JIT, users of UI package need to point Tailwind config to the node_modules.

Of course, it requires users to use Tailwind. If we want to change, we need to update the build process to export the JS files along with a CSS. This file must collect all styles from all components because otherwise users will have to import button.css, table.css, etc.

@TalDerei
Copy link
Contributor Author

@grod220 this pr follows the suggested compartmentalization defined in the issue description penumbra-zone/web#1336. The corresponding web/ui pr is penumbra-zone/web#1342. Should this PR specifically be closed in favor of keeping the UI package in this repo, following L's suggestions?

@grod220
Copy link
Contributor

grod220 commented Jun 21, 2024

@TalDerei penumbra-zone/web#1336 really just focuses on organizing @penumbra-zone/ui so that base level primitives can be exported to anticipated consumers. Radiant has a goal of making updated UI primitives with the dex/block explorer in mind especially (given they don't really look related to penumbra at the moment).

But the question with this PR is: should Prax also consume things like <Button>, <Card>, etc also from this shared library? And it really comes down to A) what in the new UI components will Prax actually use? will Prax look completely different and therefore the shared library isn't really relevant? and B) how can they be used with security in mind? In short, that probably means:

  • For now, we live with the UI package fork (perhaps there's some stuff we can delete in there). Prax UI will be out of sync with minifront + ecosystem base-level components.
  • Post new brand guidelines, Radiant will also work on Prax design updates. At that point, it will become clearer what in the shared library that Prax 2.0 should use.
  • And then we can either:
    • Consume those specific base-level components (like buttons) via NPM on a pinned version
    • Or copy them over again

@TalDerei
Copy link
Contributor Author

@TalDerei penumbra-zone/web#1336 really just focuses on organizing @penumbra-zone/ui so that base level primitives can be exported to anticipated consumers.

sounds great! closing in favor of follow-up work that will maintain and clean up the UI package fork

@TalDerei TalDerei closed this Jun 23, 2024
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