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

Update Button prop name type to variant #18774

Merged
merged 10 commits into from
Apr 26, 2023
Merged

Update Button prop name type to variant #18774

merged 10 commits into from
Apr 26, 2023

Conversation

hakeemullahjan
Copy link
Contributor

@hakeemullahjan hakeemullahjan commented Apr 24, 2023 •
edited
Loading

Explanation

In this PR, I worked on issue #18693, I had to replace the prop value from type to variant to make it more consistent with other components ui/components/component-library and luckily this issue was assigned to me.
All files under this directory ui/components/component-library/.. have been changed. Following changes are:

  • type to variant
  • BUTTON_TYPES to BUTTON_VARIANTS
  • Comments
  • Imports & Exports
  • Test Cases
  • Storybook components and its documentations
  • README.mdx

ui/components/component-library/button/button.constants.js
ui/components/component-library/button/button.js
ui/components/component-library/button/button.stories.js
ui/components/component-library/button/button.test.js
ui/components/component-library/button/index.js
ui/components/component-library/button/README.mdx
ui/components/component-library/index.js

ui/components/app/modals/hold-to-reveal-modal/hold-to-reveal-modal.js
ui/components/app/terms-of-use-popup/terms-of-use-popup.js
ui/components/institutional/compliance-settings/compliance-settings.js
ui/components/multichain/network-list-menu/network-list-menu.js
ui/pages/create-account/connect-hardware/index.js
ui/pages/institutional/confirm-add-custodian-token/confirm-add-custodian-token.js
ui/pages/institutional/confirm-add-institutional-feature/confirm-add-institutional-feature.js
ui/pages/keychains/reveal-seed.js
ui/components/app/snaps/snap-version/snap-version.js
ui/components/institutional/custody-confirm-link-modal/custody-confirm-link-modal.js
ui/pages/settings/snaps/view-snap/view-snap.js

Screenshots/Screencaps

[README] Button

Before

button-readme-before

After

button-readme-after

[Storybook] Components / ComponentsLibray / Button - Default

Before

button-default-before

After

button-default-after

[Storybook] Components / ComponentsLibray / Button - Type to Variant

Before

button-type-before

After

button-variant-after

[Storybook] ui/components/app/terms-of-use-popup/terms-of-use-popup.js

Before

terms-of-use-popup js1

After

terms-of-use-popup js2

[Storybook] ui/components/institutional/compliance-settings/compliance-settings.js

Before

compliance-settings js1

After

compliance-settings js2

[Storybook] ui/components/multichain/network-list-menu/network-list-menu.js

Before

network-list-menu js1

After

network-list-menu js2

[Storybook] ui/pages/create-account/connect-hardware/index.js

Before

connect-hardware1

After

connect-hardware2

[Storybook] ui/pages/institutional/confirm-add-custodian-token/confirm-add-custodian-token.js

Before

confirm-add-custodian-token js1

After

confirm-add-custodian-token js2

[Storybook] ui/pages/institutional/confirm-add-institutional-feature/confirm-add-institutional-feature.js

Before

confirm-add-institutional-feature js1

After

confirm-add-institutional-feature js2

[Storybook] ui/pages/keychains/reveal-seed.js

Before

reveal-seed js1

After

reveal-seed js2

Manual Testing Steps

yarn storybook then open http://localhost:6006/ and start comparing above changed components with https://metamask.github.io/metamask-storybook/?path=/story/components-componentlibrary-button--default-story

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@hakeemullahjan hakeemullahjan requested a review from a team as a code owner April 24, 2023 12:40
@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2023 •
edited
Loading

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@hakeemullahjan hakeemullahjan mentioned this pull request Apr 24, 2023
Closed
@hakeemullahjan
Copy link
Contributor Author

#18693

@hakeemullahjan
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@hakeemullahjan
Copy link
Contributor Author

@georgewrmarshall I am done with all requested changes, please can you or someone else review this PR?

@georgewrmarshall georgewrmarshall requested review from georgewrmarshall and garrettbear and removed request for montelaidev April 24, 2023 21:58
garrettbear
garrettbear previously approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@garrettbear garrettbear left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

georgewrmarshall
georgewrmarshall reviewed Apr 25, 2023
View reviewed changes
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Hey @hakeemullahjan, these changes look great! One small detail is that the BUTTON_VARIANT should be singular without the S? Could you please update accordingly?

@hakeemullahjan
Copy link
Contributor Author

Hey @hakeemullahjan, these changes look great! One small detail is that the BUTTON_VARIANT should be singular without the S? Could you please update accordingly?

Yeah sure, No problem.

@hakeemullahjan
Copy link
Contributor Author

@georgewrmarshall @garrettbear done.

DDDDDanica
DDDDDanica previously approved these changes Apr 25, 2023
View reviewed changes
@hakeemullahjan
Copy link
Contributor Author

I have double-checked all the files which are using the button from the component-library then ran test cases locally and pass all test cases. @DDDDDanica @legobeat @garrettbear @georgewrmarshall can you approve the latest changes to verify CircleCI GitHub actions?

@georgewrmarshall georgewrmarshall added the team-design-system All issues relating to design system in Extension label Apr 26, 2023
georgewrmarshall
georgewrmarshall reviewed Apr 26, 2023
View reviewed changes
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution 🙏

This was referenced Apr 26, 2023
Closed
Closed
@brad-decker brad-decker merged commit 1f0c0d0 into MetaMask:develop Apr 26, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DDDDDanica DDDDDanica DDDDDanica approved these changes

@georgewrmarshall georgewrmarshall georgewrmarshall approved these changes

@garrettbear garrettbear garrettbear left review comments

Assignees
No one assigned
Labels
external-contributor team-design-system All issues relating to design system in Extension
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hakeemullahjan @georgewrmarshall @DDDDDanica @garrettbear @brad-decker @legobeat