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

Migrate ButtonBase to TS #19067

Closed

Conversation

thebinij
Copy link
Contributor

@thebinij thebinij commented May 9, 2023

Explanation

Manual Testing Steps

No functional changes

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
  • 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.

@thebinij thebinij requested a review from a team as a code owner May 9, 2023 10:56
@thebinij thebinij requested a review from micaelae May 9, 2023 10:56
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

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.

@thebinij thebinij changed the title Migrage ButtonBase to TS Migrate ButtonBase to TS May 9, 2023
@thebinij
Copy link
Contributor Author

thebinij commented May 9, 2023

Hi @georgewrmarshall, unfortunately my Old PR has been auto-closed. However, I have implemented your suggestions in this PR. Please review this. Thanks!

@georgewrmarshall georgewrmarshall requested review from georgewrmarshall and garrettbear and removed request for micaelae May 9, 2023 22:23
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.

Code is looking good. Left one more question

@georgewrmarshall georgewrmarshall added the team-design-system All issues relating to design system in Extension label May 10, 2023
@georgewrmarshall
Copy link
Contributor

Hey @thebinij , just wanted to check in. I think we can merge this and I'll work on fixing the Box types in a separate PR just needs a rebase and resolve the conflicts. Did you still want to work on it? If so, that's great! Let us know if you need any assistance or if there are any blockers. If you're unable to continue working on it or haven't had a chance to address it, no worries! I can take it from here and carry it forward. Cheers!

@thebinij
Copy link
Contributor Author

Hi @georgewrmarshall, I am willing continue on this. I'll rebase the branch and resolve any conflicts that may arise. Thanks

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #19067 (70fa427) into develop (832ce63) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 70fa427 differs from pull request most recent head f2fc41c. Consider uploading reports for the commit f2fc41c to get more accurate results

@@             Coverage Diff             @@
##           develop   #19067      +/-   ##
===========================================
- Coverage    70.38%   70.33%   -0.05%     
===========================================
  Files          961      959       -2     
  Lines        36744    36709      -35     
  Branches      9560     9553       -7     
===========================================
- Hits         25861    25819      -42     
- Misses       10883    10890       +7     
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%> (ø)

... and 15 files with indirect coverage changes

@thebinij thebinij force-pushed the fix/18886-Migrate-ButtonBase branch from d929f83 to 7c0cc82 Compare May 18, 2023 17:25
@thebinij
Copy link
Contributor Author

Hey @georgewrmarshall, could you help me resolve the failing of codecov report?

@thebinij thebinij force-pushed the fix/18886-Migrate-ButtonBase branch 2 times, most recently from f2fc41c to 4871491 Compare May 25, 2023 10:35
@thebinij thebinij force-pushed the fix/18886-Migrate-ButtonBase branch 2 times, most recently from 9f199af to fa39a59 Compare May 30, 2023 03:23
@thebinij
Copy link
Contributor Author

Hi @georgewrmarshall, could you help me fix: Error: Coverage thresholds for global NOT met ?
error

@georgewrmarshall
Copy link
Contributor

Hey @thebinij, Thanks for your time and effort in creating this pull request. As you're probably aware we have identified an issue related to the typing of the Box component, specifically with the polymorphic as prop. To address this issue, we have an open ticket at #19239 and a draft pull request at #19363 that needs to be merged before we can proceed with your PR. Thanks for your patience

@thebinij thebinij force-pushed the fix/18886-Migrate-ButtonBase branch 2 times, most recently from ee231e5 to 1556850 Compare June 14, 2023 04:22
@thebinij thebinij force-pushed the fix/18886-Migrate-ButtonBase branch from 1556850 to 00e725b Compare June 24, 2023 22:15
@thebinij
Copy link
Contributor Author

Hi @georgewrmarshall, Is there anything I can update in the PR before so you approve to merge it?

@georgewrmarshall
Copy link
Contributor

Blocked by #19572

@garrettbear
Copy link
Contributor

@thebinij thank you so much for your help! Going to close this PR in favor for this PR which is utilizing your contribution!

@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
external-contributor 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
3 participants