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

[explorer] Base UI Components #4572

Merged
merged 13 commits into from
Sep 15, 2022
Merged

[explorer] Base UI Components #4572

merged 13 commits into from
Sep 15, 2022

Conversation

Jordan-Mysten
Copy link
Contributor

@Jordan-Mysten Jordan-Mysten commented Sep 12, 2022

This introduces our new base UI components for explorer.

To accomplish this, I'm introducing Storybook, which will provide us with an easy component authoring environment outside of the explorer. These components are fully isolated which makes them easier to develop, test, and use in visual regression testing.

I also introduced our first two base component: Text and Heading. Both of these are incredibly simple, with the styles taken directly from the Figma. The naming is aligned with what is used in the figma as well, which should make it easy to grab the correct styles when using these components. I also updated the tailwind config to include the base styles, so that it's easy to get text styles correct outside of these two components, which will be important for other base components that render text but do not use the Text component. The conditional application of styles is performed with cva.

The Storybook + Vite integration is workable, but not super great. I think it'll get better as time goes on, and I'm keeping my eye on alternatives as well (i.e. ladle, and historie).


As part of this change, I'm also moving away from CSS modules, and to use Tailwind classes directly in components. This was recently enabled by our migration to Vite, and I think it's the right way to move.
Generally, atomic CSS classes (which tailwind uses) tends to result in a smaller CSS file as applications grow, and also tend to cache better (depends on bundler setup, but cache invalidation is pretty). This also makes it easier to author components, as styling is co-located with the presentational logic (this is one of the main benefits of CSS-in-JS solutions, and is a similar justification to why JSX was originally created).

Another benefit of moving away from CSS modules is that it should help us have better practices around CSS. Right now we have a lot of styling that's performed based on tags and DOM hierarchy, which aren't things that we should depend on (especially non-semantic tag styling), makes it harder to refactor, and harder to debug styling issues.

@Jordan-Mysten Jordan-Mysten changed the title [wip] Integrate storybook [wip] Base Components Sep 12, 2022
@Jordan-Mysten Jordan-Mysten changed the title [wip] Base Components [explorer] Base UI Components Sep 12, 2022
@Jordan-Mysten Jordan-Mysten marked this pull request as ready for review September 12, 2022 23:42
Copy link
Contributor

@pchrysochoidis pchrysochoidis left a comment

Choose a reason for hiding this comment

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

lgtm - maybe wait for @Andrew47 or @Jibz1 to review as well, since they are mainly working on explorer

@apburnie
Copy link
Contributor

@Jordan-Mysten , The strategy of using Class Variance Authority to underpin shared components that can then be used across the Frontend Apps is a great idea. I'm trying to understand why this PR seems to be affecting the UI when it shouldn't.

Below I've taken screenshots of what the rows in the Recent Transactions table look like in main branch and under this PR:

image

Here's also a screenshot from Address Results:

image

Besides running pnpm install is there any other required set up?

There are also conflicts in the pnpm-lock.yml file and type errors being raised by the Typechecker (tsc --noEmit) that are blocking the merge.

@Jordan-Mysten Jordan-Mysten merged commit 01a752a into main Sep 15, 2022
@Jordan-Mysten Jordan-Mysten deleted the jordan--explorer-storybook branch September 15, 2022 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants