README for new UI library #1411

jessepinho · 2024-07-03T21:10:13Z

This sets out some coding standards for the new UI library to ensure file structure, naming, etc. are consistent.

Only requesting a review from Gabe, but others should feel free to review as well.

changeset-bot · 2024-07-03T21:10:16Z

⚠️ No Changeset found

Latest commit: 6bffda8

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

packages/ui/package.json

packages/ui/readme.md

VanishMax · 2024-07-04T07:50:25Z

packages/ui/readme.md

+
+This ensures that each component is internally responsible for its own styling.
+
+Any variations of the component's appearance must be controlled via props like `variant`, `state`, etc. — not by kitchen-sink props like `className` and `style`, which allow arbitrary changes to be made that could interfere with the internal structure of the component and cause visual inconsistencies between instances of Penumbra UI components.


question: even though I agree on this idea, there are cases where className existence is important. For example, take a look at the AssetsTable Row implementation in minifront. When the TableRow with class 'group' is hovered, the inner Link changes its opacity. How can we handle such case?

Oh man, good find of an edge case... Hm.

Went down a rabbit hole trying to solve this problem, and here's what I came up with:

<TableBody> {account.balances.map((assetBalance, index) => ( <TableRow key={index}> <TableCell> <ValueViewComponent view={assetBalance.balanceView} /> </TableCell> <TableCell> <EquivalentValues valueView={assetBalance.balanceView} /> </TableCell> <TableCell> <Link className='transition md:opacity-0 [tr:hover>td>&]:opacity-100' to={getTradeLink(assetBalance)} > <Button variant='secondary' size='md'> Trade </Button> </Link> </TableCell> </TableRow> ))} </TableBody>

I've removed group entirely, and used nesting selectors to get the same effect. (I tried using group in combination with & (docs) to get the same effect, but it didn't work. I suspect I'm doing something wrong, and could fix this with a bit more work, but it didn't seem worth the time right now.)

This isn't a great solution (since it requires a very specific DOM structure), so I'll keep playing with it once I cross this particular bridge. But for now, I think we can still avoid className/style props.

Wow, didn't know it's possible with Tailwind! Thanks for showing this. It probably worth being documented

grod220

Some thoughts, but definitely think this is on the right track!

grod220 · 2024-07-05T09:05:14Z

packages/ui/readme.md

+
+This ensures that the Penumbra UI `package.json` `exports` field works as intended, so that components can be imported via `@penumbra-zone/ui/<ComponentName>`.
+
+Note that `<ComponentName>` should be replaced with the `UpperCamelCase` component name — e.g., `./components/v2/LoadingIndicator/index.tsx`.


In the past i've run into issues with using capitalization in file names as different operating systems handle case sensitivity in different ways. But maybe not a big deal.

As long as people import it using the same capitalization at the file name, it should work across all systems, right? (I wanted to do this change both A) to make imports a little less annoying to type, and B) because it seems to be the norm across other libraries like MUI — example.)

grod220 · 2024-07-05T09:11:22Z

packages/ui/readme.md

+### Components must be the default export of the component file.
+
+This allows them to be imported as the default, rather than destructuring an object. It also encourages UI library maintainers to only export one component per file.


I'm not sure this represents the most current thinking on exporting. Last year there was a flurry of articles written around the same time (e.g. https://dev.to/phuocng/avoid-using-default-exports-a1c) and lots of discussion on twitter on how default exports are kinda bad. I find their reasoning compelling.

Hm, a few thoughts:

I don't have a strong opinion about this one way or the other, so I'm happy to change it.

It seems that a lot of the discussions of this issue are claiming to be about named vs. default exports, when they should be about named vs. unnamed exports. See Addy Osmani's tweet here, which encourages people to name their exports (whether they are default exports or not).

A named default export (OK to use, in my opinion):

export default function MyComponent() {}

An unnamed default export (bad to use, in my opinion):

export default () => {}

I haven't had the experience that others are describing, where IDEs don't work well with default exports. In my experience with VSCode, if I want to import a default-exported MyComponent, I type <MyComponent in my JSX markup, and it automatically knows to import it. If I later rename MyComponent, it also updates everywhere in the codebase that imports MyComponent, even though it's the default export (since, after all, it is still a named function). It could just be that VSCode is especially smart about this, but I doubt it — I think it's a language-level thing, since MyComponent is actually a named function that happens to be exported as the default.

If you feel strongly about this, though, I can definitely remove this guideline.

I've pushed f01b893 to address this; but pending your response to my comment above, I can revert it back to my original guideline.

grod220 · 2024-07-05T09:13:14Z

packages/ui/readme.md

+}
+```
+
+### Components should include Storybook stories.


Storybook comes with a testing framework right? or snapshotting?

Should we require test coverage for these new components?

Ah, let me write something up about testing.

Storybook does have integrations with screenshot (not snapshot) testing, but the only one I know of is Chromatic, which is a bit pricey. We could consider that in the future, but for now, I should at least include a point about unit tests.

Done: 6bffda8

VanishMax

This version is well-polished! Approving since all my concerns are noted but it's probably worth to wait for @grod220 to approve too

jessepinho · 2024-07-05T20:19:25Z

Thanks @VanishMax ! @grod220 please weigh in when you have a chance

grod220

Ship it 🚢

jessepinho force-pushed the jessepinho/ui-readme-web-82 branch from 192433b to 45c35fd Compare July 3, 2024 22:54

Update the @penumbra-zone/ui README in advance of the v2 library

c85755f

jessepinho force-pushed the jessepinho/ui-readme-web-82 branch from 45c35fd to c85755f Compare July 3, 2024 22:56

jessepinho commented Jul 3, 2024

View reviewed changes

packages/ui/package.json Outdated Show resolved Hide resolved

jessepinho added 2 commits July 3, 2024 16:02

Add naming convention

d400cd9

Add separate point about JSDoc-style comments

28f01bb

jessepinho changed the title ~~WIP: README for new UI library~~ README for new UI library Jul 3, 2024

jessepinho marked this pull request as ready for review July 3, 2024 23:19

jessepinho requested a review from grod220 July 3, 2024 23:19

VanishMax reviewed Jul 4, 2024

View reviewed changes

grod220 reviewed Jul 5, 2024

View reviewed changes

jessepinho added 5 commits July 5, 2024 12:16

Don't use default exports

f01b893

Export the component props interface

3785649

Change directory of v2 components

f055047

Tweak a couple guidelines

c915893

Add note about testing

6bffda8

VanishMax approved these changes Jul 5, 2024

View reviewed changes

grod220 approved these changes Jul 8, 2024

View reviewed changes

jessepinho merged commit 11893fc into main Jul 8, 2024
6 checks passed

jessepinho deleted the jessepinho/ui-readme-web-82 branch July 8, 2024 22:52

VanishMax mentioned this pull request Sep 2, 2024

Move toward a "black box" component library model for packages/ui #432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

README for new UI library #1411

README for new UI library #1411

jessepinho commented Jul 3, 2024 •

edited

Loading

changeset-bot bot commented Jul 3, 2024 •

edited

Loading

VanishMax Jul 4, 2024

jessepinho Jul 5, 2024 •

edited

Loading

VanishMax Jul 5, 2024

grod220 left a comment

grod220 Jul 5, 2024

jessepinho Jul 5, 2024

grod220 Jul 5, 2024

jessepinho Jul 5, 2024 •

edited

Loading

jessepinho Jul 5, 2024

grod220 Jul 5, 2024

jessepinho Jul 5, 2024

jessepinho Jul 5, 2024

VanishMax left a comment

jessepinho commented Jul 5, 2024

grod220 left a comment


		This ensures that each component is internally responsible for its own styling.

		Any variations of the component's appearance must be controlled via props like `variant`, `state`, etc. — not by kitchen-sink props like `className` and `style`, which allow arbitrary changes to be made that could interfere with the internal structure of the component and cause visual inconsistencies between instances of Penumbra UI components.


		This ensures that the Penumbra UI `package.json` `exports` field works as intended, so that components can be imported via `@penumbra-zone/ui/<ComponentName>`.

		Note that `<ComponentName>` should be replaced with the `UpperCamelCase` component name — e.g., `./components/v2/LoadingIndicator/index.tsx`.

		### Components must be the default export of the component file.

		This allows them to be imported as the default, rather than destructuring an object. It also encourages UI library maintainers to only export one component per file.

README for new UI library #1411

README for new UI library #1411

Conversation

jessepinho commented Jul 3, 2024 • edited Loading

changeset-bot bot commented Jul 3, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

jessepinho Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessepinho Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanishMax left a comment

Choose a reason for hiding this comment

jessepinho commented Jul 5, 2024

grod220 left a comment

Choose a reason for hiding this comment

jessepinho commented Jul 3, 2024 •

edited

Loading

changeset-bot bot commented Jul 3, 2024 •

edited

Loading

jessepinho Jul 5, 2024 •

edited

Loading

jessepinho Jul 5, 2024 •

edited

Loading