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.

Sign up for GitHub

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

Jump to bottom

feat: Tailwind-based UI package #1891

Merged
merged 1 commit into from
Nov 8, 2024
Merged

feat: Tailwind-based UI package #1891

merged 1 commit into from
Nov 8, 2024

Conversation

VanishMax
Copy link
Contributor

@VanishMax VanishMax commented Nov 1, 2024 •
edited
Loading

Followed up by #1902

The Tailwind-based UI library.

Vite, in this case, builds the library code and produces simple JavaScript that takes React's createElement and applies the right classes to it. The styling responsibility for now is on the consumer's side. However, I exported one function for simple Tailwind configuration of consumer's tailwind.config.ts:

import { withPenumbra } from '@penumbra-zone/ui-tailwind/theme';

export default withPenumbra({
  content: [
    './app/**/*.{js,ts,jsx,tsx,mdx,css}',
    './src/**/*.{js,ts,jsx,tsx,mdx,css}',
  ],
});

Components list. Agenda: ✅ – implemented, 🟡 – not implemented (not needed for now), ❌ – i decided not to implement it.

  1. ⚠️AccountSelector
  2. ✅AddressViewComponent
  3. ✅AssetIcon
  4. ✅AssetSelector
  5. ✅Button
  6. ❌ButtonGroup
  7. ✅Card
  8. ❌CharacterTransition
  9. ✅ConditionalWrap
  10. ✅CopyToClipboardButton
  11. ✅Density
  12. ✅️Dialog
  13. ✅Display
  14. ✅DropdownMenu
  15. 🟡FormField
  16. ✅Grid
  17. ✅Icon
  18. ✅Identicon
  19. ❌IsAnimatingProvider
  20. ✅MenuItem
  21. ❌PenumbraUIProvider
  22. ✅Pill
  23. ✅Popover
  24. ✅Progress
  25. 🟡SegmentedControl
  26. 🟡Slider
  27. 🟡SwapInput
  28. ✅Table
  29. ✅Tabs
  30. ✅Text
  31. ✅TextInput
  32. ✅Toast
  33. 🟡Toggle
  34. ✅Tooltip
  35. 🟡ValueInput
  36. ✅ValueView
  37. ✅WalletBalance

Future work

  1. Improve withPenumbra hook to cover all config merging cases
  2. Add tests to components, move old tests back and make them work
  3. Export a .css file with all compiled Tailwind classes, so consumers wouldn't need Tailwind in their projects
  4. Implement other UI components that were left undone for now

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 1, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 4e49ce9

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

@VanishMax VanishMax self-assigned this Nov 1, 2024
VanishMax
VanishMax commented Nov 4, 2024
View reviewed changes
packages/ui-tailwind/src/utils/color.ts
* be produced from the `getThemeColorClass` function. */
export const COLOR_CLASS_MAP: Record<ThemeColor, [string, string]> = {
'neutral.main': ['text-neutral-main', 'bg-neutral-main'],
'neutral.light': ['text-neutral-light', 'bg-neutral-light'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this color mapping is needed for Tailwind's compiler. It needs to see static classes, dynamic ones fail to be parsed

@VanishMax VanishMax mentioned this pull request Nov 4, 2024
Merged
@VanishMax VanishMax requested a review from a team November 7, 2024 13:11
@VanishMax VanishMax marked this pull request as ready for review November 7, 2024 13:23
@VanishMax VanishMax mentioned this pull request Nov 7, 2024
Merged
grod220
grod220 approved these changes Nov 7, 2024
View reviewed changes
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.

What a heroic effort on this 🎊 . Huge win getting this fixed.

packages/ui-tailwind/src/Card/index.stories.tsx

import { Card } from '.';

// import storiesBg from './storiesBg.jpg';
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove

packages/ui-tailwind/src/Card/index.stories.tsx

render: function Render({ as, title }) {
const [tab, setTab] = useState('one');
// const [textInput, setTextInput] = useState('');
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove

.changeset/wise-toes-bathe.md Outdated
Comment on lines 1 to 3
---
'@penumbra-zone/ui': major
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally we can:

  • rename our old package to @penumbra-zone/ui-deprecated and set it to private
  • name the new package @penumbra-zone/ui, set it as the old version 12.4.0, and keep this major bump for that. After changeset versions everything, it should publish the new stuff.

However, this probably requires changing all the import paths. Maybe good for a next PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did it here: #1902

packages/ui-tailwind/src/theme/tailwind-config.ts
Comment on lines +55 to +67
export const withPenumbra = (config: Config): Config => {
return {
...config,
content: composeContent(config.content),
theme: {
...config.theme,
extend: {
...tailwindConfig.theme.extend,
...config.theme?.extend,
},
},
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: does this guarantee no conflicts with the consumers existing styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably no, ideally it should recursively merge all nested fields, especially in the theme.extend object. That is on my list of future improvements to the library along with exporting the whole .css file of the library

@VanishMax
Copy link
Contributor Author

@grod220 do you have the idea why the tests are failing in the minifront? Seems like it is triggered by your changes

https://github.com/penumbra-zone/web/actions/runs/11724483746/job/32658624212?pr=1891

@grod220
Copy link
Collaborator

grod220 commented Nov 7, 2024

The "multiple copies of react" issue sounds like you are bundling react with the tailwind package. Think that means you need to make it a peerDep and not a main dependency.

@VanishMax VanishMax merged commit 571a818 into main Nov 8, 2024
6 checks passed
@VanishMax VanishMax deleted the feat/ui-tailwind branch November 8, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grod220 grod220 grod220 approved these changes

Assignees

@VanishMax VanishMax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@VanishMax @grod220