-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Updating Text component to use TS Box #19949
Updating Text component to use TS Box #19949
Conversation
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. |
9ef3eb6
to
2e5a170
Compare
/** | ||
* @deprecated This version of the `<Text />` component has been deprecated | ||
* Use `import { Text } from '../../component-library';` instead | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding deprecation notice to old version of Text
component
Builds ready [2e5a170]
Page Load Metrics (1608 ± 55 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏🏻 LGTM
b5ce5e1
2e5a170
to
b5ce5e1
Compare
b5ce5e1
to
75cf2f6
Compare
Builds ready [75cf2f6]
Page Load Metrics (1755 ± 109 ms)
Bundle size diffs
|
75cf2f6
to
858b0cb
Compare
858b0cb
to
5213d0a
Compare
Builds ready [5213d0a]
Page Load Metrics (1527 ± 32 ms)
Bundle size diffs
|
Codecov Report
@@ Coverage Diff @@
## develop #19949 +/- ##
===========================================
- Coverage 69.47% 69.47% -0.01%
===========================================
Files 988 988
Lines 37313 37306 -7
Branches 9989 9986 -3
===========================================
- Hits 25923 25916 -7
Misses 11390 11390
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Note: I only reviewed the files under snaps ownership since this PR already had multiple approvals.
Explanation
Currently, the
Text
component is using the deprecated JavaScript version of theBox
component. This blocks the TypeScript migration of the extension AND causes issues with the polymorphicas
prop. This PR updates theText
component to use the TypeScript version of theBox
and moves the old version ofText
totext/deprecated
folder and updates all imports across the extension.Why not just update the
Text
component withBox
if the API is the same?I have another PR open doing just that but because of undiagnosable e2e test failures, I have resorted to this route. I will then subsequently update all import paths folder by folder to point to the updated component. This has proven to be a successful method in the past(thanks @brad-decker and @DDDDDanica 😁). It reduces the impact on engineers and merge conflicts and will help get to the bottom of the e2e test failures if they exist or if it's just an annoying timeout.
This PR also unblocks 9 contributor PRs migrating components to TypeScript
Screenshots/Screencaps
Before
Text
component indevelop
branch in storybookbefore.mov
Deprecation message for deprecated
Text
componentAfter
Showing updated
Text
component in storybook using the TS version of theBox
component. Showing no visual regressions and all stories and controls are working as expected. I have made some small updates to more easily find the replaceTypography
withText
section and linked the issueafter.mov
Manual Testing Steps
To check storybook
Text
in the search panelTo check all import paths have been updated correctly.
Text
components are slightly different one uses the TypeScriptBox
and has the classNamemm-box
vs the JavaScript versionbox-
. SO this means that any snapshot tests will pick this up. There should be NO snapshot test updates with this update in this PRI also did a search for
<Text
and part of the new import pathtext/deprecated
to compare the number of files165
files vs164
files for.js
files with the 1 difference being a comment in a deprecation message11
files vs11
files for.tsx
filesPre-merge author checklist
Pre-merge reviewer checklist
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.