-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: alignment of Snaps icons within ButtonLink/SnapUIButton component #14243
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
147edd5
9d280d9
252dee3
bde49a6
bdbd6b8
48bbe09
01b93a3
5862765
0718eeb
dc24a00
3289ecd
2ef9b13
cf494ed
795ab2a
8b8c459
5779a82
6628758
87af066
afe910e
fdbfaa4
c29d582
39fce31
9fcc492
6cd0137
97b23b5
e1b84cc
c76a6d8
b03005d
82004b0
51e9d20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,6 @@ | |
import { | ||
TouchableOpacity, | ||
StyleSheet, | ||
View, | ||
ViewStyle, | ||
StyleProp, | ||
TouchableOpacityProps, | ||
|
@@ -117,10 +116,6 @@ | |
); | ||
} | ||
|
||
if (isIcon) { | ||
return <View style={styles.content}>{children}</View>; | ||
} | ||
|
||
if (typeof children === 'string') { | ||
return ( | ||
<Text color={color} variant={textVariant}> | ||
|
@@ -129,6 +124,36 @@ | |
); | ||
} | ||
|
||
if (React.isValidElement(children) && children.props?.sections) { | ||
try { | ||
const modifiedSections = children.props.sections.map((section: any) => { | ||
if (section.element === 'RNText') { | ||
return { | ||
...section, | ||
props: { | ||
...section.props, | ||
style: { | ||
...section.props?.style, | ||
color: colors.primary.default, | ||
}, | ||
}, | ||
}; | ||
} | ||
return section; | ||
}); | ||
|
||
return React.cloneElement( | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
children as React.ReactElement<{ sections: any }>, | ||
{ | ||
sections: modifiedSections, | ||
}, | ||
); | ||
} catch (error) { | ||
console.log('Error modifying sections:', error); | ||
} | ||
} | ||
|
||
return children; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Let me take a look and make some changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be resolved now |
||
}; | ||
|
||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theSnapUIButton.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 thebutton.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 themapToTemplate
function?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.
We already have access to the theme values in the mappers! For reference:
metamask-mobile/app/components/Snaps/SnapUIRenderer/components/section.ts
Line 7 in a9ccd49
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.
Awesome, this might work then