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

Fix/buttonbase ts update #20060

Merged
merged 16 commits into from
Jul 25, 2023
Merged

Fix/buttonbase ts update #20060

merged 16 commits into from
Jul 25, 2023

Conversation

garrettbear
Copy link
Contributor

@garrettbear garrettbear commented Jul 17, 2023

Explanation

Update ButtonBase to use TS. No visual change

*Fixes 18886

Screenshots/Screencaps

Before

Screenshot 2023-07-18 at 8 40 11 AM

After

Screenshot 2023-07-18 at 8 40 45 AM

Manual Testing Steps

yarn jest ui/components/component-library/button-base

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.

@github-actions
Copy link
Contributor

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.

fix error added TextAlignArray

resolve deprecated

fix lint error by removing textalign prop from textProps

button base box updates

update buttonbase to use new box and TS
@metamaskbot
Copy link
Collaborator

Builds ready [1083b90]
Page Load Metrics (1747 ± 66 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1203031583818
domContentLoaded15782037174713866
load15782037174713866
domInteractive15782037174713866
Bundle size diffs
  • background: 0 bytes
  • ui: -960 bytes
  • common: 0 bytes

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #20060 (779ac5e) into develop (74a645e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop   #20060   +/-   ##
========================================
  Coverage    69.37%   69.37%           
========================================
  Files          987      986    -1     
  Lines        37292    37296    +4     
  Branches     10012    10020    +8     
========================================
+ Hits         25869    25873    +4     
  Misses       11423    11423           
Impacted Files Coverage Δ
ui/components/component-library/text/text.types.ts 100.00% <ø> (ø)
ui/helpers/constants/design-system.ts 100.00% <ø> (ø)
...ents/component-library/button-base/button-base.tsx 100.00% <100.00%> (ø)

@garrettbear garrettbear marked this pull request as ready for review July 18, 2023 06:04
@garrettbear garrettbear requested a review from a team as a code owner July 18, 2023 06:04
DDDDDanica
DDDDDanica previously approved these changes Jul 18, 2023
@DDDDDanica
Copy link
Contributor

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [3ab4438]
Page Load Metrics (1589 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1152721423416
domContentLoaded1522183915887335
load1523183915897335
domInteractive1522183915887335
Bundle size diffs
  • background: 0 bytes
  • ui: -975 bytes
  • common: 0 bytes

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.

Looking good. Some things I noticed while testing storybook

  • Text is blue on hover when the as prop is an a tag
Screenshot 2023-07-18 at 7 17 56 PM - `block` dont behave as I expected when `as` prop is an `a` tag Screenshot 2023-07-18 at 7 19 24 PM - When `block` is true icons seem to be misaligned Screenshot 2023-07-18 at 7 22 03 PM

These may have been existing bugs so if you want to create another issue and fix them there that's fine too

ui/components/component-library/text/text.types.ts Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [bce2295]
Page Load Metrics (1510 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint99156122157
domContentLoaded1364165215107235
load1364165215107235
domInteractive1364165215107235
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -1001 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

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.

Need to figure out what is happening with this type error. There shouldn't be any listing issues when the component is being used in the stories. Might be a similar issue to the Checkbox?

Screenshot 2023-07-19 at 2 24 00 PM

@metamaskbot
Copy link
Collaborator

Builds ready [79134d5]
Page Load Metrics (1885 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1212261632612
domContentLoaded15992114188511957
load15992114188511957
domInteractive15992114188511957
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -983 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

@georgewrmarshall georgewrmarshall added the team-design-system All issues relating to design system in Extension label Jul 19, 2023
@garrettbear
Copy link
Contributor Author

Need to figure out what is happening with this type error. There shouldn't be any listing issues when the component is being used in the stories. Might be a similar issue to the Checkbox?

Screenshot 2023-07-19 at 2 24 00 PM

@georgewrmarshall that error is inconsistent because I don't have it.

Screenshot 2023-07-20 at 8 54 45 AM

@metamaskbot
Copy link
Collaborator

Builds ready [b428eb9]
Page Load Metrics (1654 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1131971462110
domContentLoaded14671942165412057
load14671942165412058
domInteractive14671942165412057
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -983 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

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.

Still seems like you can pass wrong attributes down to the root HTML element

* fixing as issue

* strict prop fixes for george
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 with one nit

@metamaskbot
Copy link
Collaborator

Builds ready [1c975ea]
Page Load Metrics (1544 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint114176134189
domContentLoaded1409170315438440
load1409170315448440
domInteractive1409170315438440
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -919 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [17988bd]
Page Load Metrics (1553 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint108169133189
domContentLoaded1426169415527737
load1427169415537737
domInteractive1426169415527737
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -931 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

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 🔥🔥🔥

  • Tested changing as prop between a and button and adding different HTML element specific props like externalLink and disabled. No incorrect HTML attributes were added and functionality and style was still applied. ✅
  • Checked stories and documentation ✅
  • Pulled branch to ensure no linting issues in stories or test files ✅
  • No use of deprecated components or code e.g consts, js components etc ✅

@garrettbear garrettbear mentioned this pull request Jul 24, 2023
6 tasks
@metamaskbot
Copy link
Collaborator

Builds ready [779ac5e]
Page Load Metrics (1652 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint110182134199
domContentLoaded14851864165211455
load14851864165211455
domInteractive14851864165211455
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -931 Bytes (-0.01%)
  • common: 0 Bytes (0.00%)

@garrettbear garrettbear merged commit 3f27d01 into develop Jul 25, 2023
@garrettbear garrettbear deleted the fix/buttonbase-ts-update branch July 25, 2023 16:05
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2023
@metamaskbot metamaskbot added the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Jul 25, 2023
@Gudahtt Gudahtt added release-11.1.0 Issue or pull request that will be included in release 11.1.0 and removed release-10.36.0 Issue or pull request that will be included in release 10.36.0 labels Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.1.0 Issue or pull request that will be included in release 11.1.0 team-design-system All issues relating to design system in Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate components to TS: ButtonBase
7 participants