-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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/18885 migrate avatar token #19080
Fix/18885 migrate avatar token #19080
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. |
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.
Hey @thebinij, I've checked out your branch and tried a few things to get the types to work but I can't seem to find a reliable pattern. I think for now we just make AvatarBaseProps
accept all variant size types as a union type for now and update it in the future once we find a scalable pattern to use.
Codecov Report
@@ Coverage Diff @@
## develop #19080 +/- ##
===========================================
- Coverage 69.46% 69.45% -0.01%
===========================================
Files 983 982 -1
Lines 37299 37297 -2
Branches 10025 10026 +1
===========================================
- Hits 25908 25904 -4
- Misses 11391 11393 +2
|
ui/components/component-library/avatar-token/avatar-token.test.tsx
Outdated
Show resolved
Hide resolved
8ee5ca4
to
dea4413
Compare
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 |
fix lint fixing error replace BoxTextAlign with TextAlign using AvatarTokenSize fix avatar-base-type
fix lint fixing error replace BoxTextAlign with TextAlign using AvatarTokenSize fix avatar-base-type Added union AvatarTokenSize TokenProps extends BaseProps remove children prop from BaseProps replace deprecated lint fix
137259d
to
b0468e8
Compare
@@ -13,7 +13,7 @@ export enum AvatarNetworkSize { | |||
* Props for the AvatarNetwork component | |||
*/ | |||
export interface AvatarNetworkStyleUtilityProps | |||
extends Omit<AvatarBaseStyleUtilityProps, 'size'> { | |||
extends Omit<AvatarBaseStyleUtilityProps, 'size' | 'children'> { |
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.
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.
I think this makes sense to my limited TypeScript knowledged brain. We are adjusting the types at the same level we are increasing the specificity of the component. We can omit the children
prop because we are supplying the children using the <img />
element or fallbackString
I think it makes more sense that making children optional on the AvatarBase level
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.
Looks alll G to ME! Thanks for your contribution @thebinij and for supporting @garrettbear 💯
Explanation
Screenshots/Screencaps
Before
before.mp4
After
after.mp4
Manual Testing Steps
No functional changes
Pre-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.