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

refactor: use consistent button component #889

Merged
merged 3 commits into from
Nov 1, 2023
Merged

refactor: use consistent button component #889

merged 3 commits into from
Nov 1, 2023

Conversation

dohaki
Copy link
Contributor

@dohaki dohaki commented Oct 30, 2023 •
edited
Loading

Based #892 because I used Storybook for implementing the component.

This PR does the following:

  • Introduction of new consistent <Button /> component
    • While working on the new UI update, I again noticed that we don't have a consistent way of implementing buttons. Sometimes, we used the legacy component, and other times the V2 component. The V2 component was also inconsistent with the Figma designs and had a few visual bugs. This PR therefore adds a new component that uses the consistent color design token of PR refactor: align global css vars + remove some legacy code #888.

Closes ACX-1646

@vercel
Copy link

vercel bot commented Oct 30, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
frontend-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 1, 2023 2:05pm
goerli-frontend-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 1, 2023 2:05pm

@linear
Copy link

linear bot commented Oct 30, 2023

ACX-1644 Fix storybook

ACX-1646 Refactor button components

james-a-morris
james-a-morris reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

This is a huge diff. Can you outline a few locations in the code where I should drill down on?

@dohaki
Copy link
Contributor Author

dohaki commented Oct 30, 2023

This is a huge diff. Can you outline a few locations in the code where I should drill down on?

It's actually not that huge! Most of the diffs are dependency changes in the lock file.

I can try to split it out if you want.

Base automatically changed from refactor-css-vars to master October 31, 2023 08:47
@dohaki dohaki force-pushed the refactor-button-component branch from d0faf1e to 7737ae0 Compare October 31, 2023 08:56
@dohaki dohaki changed the base branch from master to bump-storybook October 31, 2023 08:58
@dohaki dohaki changed the title refactor: use consistent button component + fix storybook refactor: use consistent button component Oct 31, 2023
@dohaki
Copy link
Contributor Author

dohaki commented Oct 31, 2023

This is a huge diff. Can you outline a few locations in the code where I should drill down on?

@james-a-morris I split up this PR into #892 and this one. So you can review #892 first and then this one.

@dohaki dohaki requested a review from james-a-morris October 31, 2023 14:20
james-a-morris
james-a-morris reviewed Oct 31, 2023
View reviewed changes
src/components/Button/Button.tsx Outdated
Comment on lines 38 to 39
height: "4rem",
padding: "0rem 2.5rem",
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we begun to switch to rem instead of px for height/padding values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that in Figma we now use rem instead of px and I am assuming that Tim chose this setting for a reason. Converting these to px in your head when implementing the designs also doesn't really make sense to me, so I kept rems here as well.

I know that this leads to a bit of inconsistency but I think moving to rem makes sense. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah wait, just checked again. This is a setting on my side that led to this 😓 I think in that case it makes sense to use px for now. And move over to rem later if we decide to do so

james-a-morris
james-a-morris reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

Added a few comments. I don't notice any regressions when testing via the preview link.

@dohaki dohaki requested a review from james-a-morris October 31, 2023 15:47
@dohaki dohaki mentioned this pull request Nov 1, 2023
Merged
james-a-morris
james-a-morris reviewed Nov 1, 2023
View reviewed changes
src/components/Button/Button.tsx
export const UnstyledButton = styled(BaseButton)`
border: none;
padding: 0;
height: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use fit-content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are the arguments for using fit-content instead of auto?

james-a-morris
james-a-morris approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

This looks good to me. I don't have much else to add & everything works well on the local storybook instance I loaded.

@dohaki should we look at maybe re-introducing chromatic?

@dohaki
Copy link
Contributor Author

dohaki commented Nov 1, 2023

This looks good to me. I don't have much else to add & everything works well on the local storybook instance I loaded.

@dohaki should we look at maybe re-introducing chromatic?

Yea we could. I don't recall why we stopped using it 🤔

Base automatically changed from bump-storybook to master November 1, 2023 13:46
@dohaki dohaki force-pushed the refactor-button-component branch from 454ca90 to 854274e Compare November 1, 2023 13:47
@dohaki dohaki merged commit a955c42 into master Nov 1, 2023
6 checks passed
@dohaki dohaki deleted the refactor-button-component branch November 1, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james-a-morris james-a-morris james-a-morris approved these changes

@amateima amateima amateima approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dohaki @amateima @james-a-morris