Update Text to use TS version of Box#19572
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. |
| * Additional props to pass to the `Text` component used for the `title` text | ||
| */ | ||
| titleProps: PropTypes.shape(Text.PropTypes), | ||
| titleProps: PropTypes.object, |
There was a problem hiding this comment.
Unfortunately using Text.propTypes breaks so need to update proptypes to use generic object while during our migration process
There was a problem hiding this comment.
Is there a task captured for returning to PropTypes?
There was a problem hiding this comment.
Not a particular issues addressing the propTypes but is it possible to use types from a TypeScript component with PropTypes. The issue that would fix this in future ins the migration of the component to TypeScript
| color: var(--color-text-default); | ||
| font-family: var(--font-family-sans); | ||
|
|
||
| &:is(strong), |
There was a problem hiding this comment.
Updating so it's based on CSS not a prop
| class="mm-box mm-text beta-header__message mm-text--body-sm mm-box--color-warning-inverse" | ||
| > | ||
| <span> | ||
|
|
f99f22f to
66a9dc5
Compare
66a9dc5 to
0339946
Compare
762ccd7 to
c348964
Compare
There was a problem hiding this comment.
Now that the CSS specificity of the Box has been fixed i.e the order of stylesheets has been updated. We should set the default by the utility prop so it this style doesn't override it
There was a problem hiding this comment.
Still dealing with a TypeScript issue that complains about spreading props. This doesn't effect the functionality of the component and doesn't block the build so I'll create a ticket to look into it. Need some more time to look into it. I think as this doesn't effect anything else we move forward to unblock migration and I'll look into it in a separate issue.
There was a problem hiding this comment.
Hmmm why isn't it blocking the build 🤔 type errors should
There was a problem hiding this comment.
Is it possible your editor's typescript process was just... slow on reading updates? I don't see the error on my end when I view this file
c348964 to
4f7c1e1
Compare
4f7c1e1 to
db05689
Compare
There was a problem hiding this comment.
This seems fine and not that someone should be changing the variant but what happens if they do? Would the variant prop error?
There was a problem hiding this comment.
Hmm yes I think this should cause a temporary prop error while this component is still in JS. There is an open contributor PR to migrate it though so once this is merged I can assist with that.
727b6bd to
837b5d2
Compare
5a7a058 to
7785e72
Compare
7785e72 to
992d5fd
Compare
830700f to
a1ef6ef
Compare
a1ef6ef to
80b0251
Compare
80b0251 to
f7ce1cf
Compare
Explanation
Currently the
Textcomponent still uses the JS version of theBoxcomponent and blocks TypeScript migration. This PR updates theTextcomponent to use the TS version of theBoxcomponent. I've also added a few Before/After screenshots in the code comments to ensure no visual regressionScreenshots/Screencaps
Before
Showing original
Textcomponent in storybook to show no visual regressionsbefore.mov
After
Showing updated
Textcomponent in storybook using the TS version of theBoxcomponent. Showing no visual regressions and all stories and controls are working as expected. I have made some small updates to more easily find the replaceTypographywithTextsection and linked the issueafter.mov
Manual Testing Steps
developand the updates in this PRTo check storybook
Textin the search panelPre-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 Boardlabel.In this case, a QA Engineer approval will be be required.