fix: alignment of Snaps icons within ButtonLink/SnapUIButton component #14243
Conversation
Daniel-Cross
added
No QA Needed
|
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. |
| ); | ||
| } | ||
|
|
||
| return children; |
There was a problem hiding this comment.
One of the problems with this is that we don't inherit the text color when complex children are used:
Also for some reason this text is now center aligned?
Can be reproed by rendering
<Box>
<Button>Foo <Icon name="arrow-right" /> Bar</Button>
</Box>There was a problem hiding this comment.
Thanks! Let me take a look and make some changes.
There was a problem hiding this comment.
This seems to be resolved now
| const isIcon = | ||
| React.isValidElement(children) && | ||
| (children.props?.type === 'Icon' || | ||
| (children.type as React.ComponentType<unknown> & { displayName?: string }) |
There was a problem hiding this comment.
What is going on here? What even is .displayName?
There was a problem hiding this comment.
Yeah this has been removed, I was experimenting with something that didn't work.
| handleEvent({ | ||
| event: UserInputEventType.ButtonClickEvent, | ||
| name, | ||
| }); | ||
|
|
||
| // Since we don't have onSubmit on mobile, the button submits the form. |
There was a problem hiding this comment.
Why are we removing this documentation?
There was a problem hiding this comment.
Re-added this comment
| return React.cloneElement( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| children as React.ReactElement<{ sections: any }>, | ||
| { | ||
| sections: modifiedSections, | ||
| }, |
There was a problem hiding this comment.
Seems simpler to try and solve this at the mapper level before we turn it into React components to prevent having to do this
There was a problem hiding this comment.
I took a look at moving this over to the button.ts file and it's possible to map the children there correctly instead of in the SnapUIButton.tsx component, however we still have to map the children again in this component to apply the correct colour to the text as we don't have access to the Theme colours, which is a React hook, and not accessible in the button.ts file.
Not really sure how to solve both the alignment issue (multiple Text component wraps) and the text colour inheritance issue at this point if we need this to be in the button.ts mapper component.
I can perhaps create some sort of theme utility that doesn't rely on react, or I can pass them through via params from the utils.ts file via the mapToTemplate function?
There was a problem hiding this comment.
however we still have to map the children again in this component to apply the correct colour to the text as we don't have access to the Theme colours, which is a React hook, and not accessible in the button.ts file.
We already have access to the theme values in the mappers! For reference:
There was a problem hiding this comment.
Awesome, this might work then
|
| /** | ||
| * Apply styles to a component based on its type | ||
| */ | ||
| function styleComponent(component: UIComponent): UIComponent { |
There was a problem hiding this comment.
I think we can avoid all of this with #14355
|
|
||
| const color = COLORS[overriddenVariant as keyof typeof COLORS]; |
There was a problem hiding this comment.
We still want to support the colors for the Snap UI button, but we may have to do it at the mapper level once #14355 lands
| disabled = false, | ||
| loading = false, | ||
| textVariant, | ||
| style, | ||
| onPress, |
Description
Snaps sometimes passes in Icons as part of a button, such as a close or swap button. They are currently wrapped in multiple
<Text/>components which pushes the alignment out. The ways to address this is to remove the text component wrappers but that would also involve making changes to the DS componentButtonLink, which would affect the rest of the app.ButtonLinkwraps any child passed in with a text component ifButtonSizeis set toAuto. When set toSm,Md, orLg, and the text component wrapper is removed from theSnapUIButtoncomponent the Icon is correctly aligned, howeverpaddingHorizonal: 16is then applied to the Icon which forces the Icon button to take up more space in text inputs and the layout in general.It currently seems the only way to address this issue is to make changes to the DS component or build a custom button component within
SnapUIButtonto fit the needs of Snaps specifically.Related issues
Fixes: MetaMask/snaps#3244
Manual testing steps
SendScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist