Migrate Kyper AttentionFilled Icon to MXUI - #356
Conversation
|
@mwclemy it looks like the warning symbol changed from grey to black. Is that intentional? |
| dangerouslySetInnerHTML={{ __html: sanitizedInstructionalText }} | ||
| data-test="instructional_text" | ||
| sx={{ marginBottom: tokens.Spacing.XSmall, ...style }} | ||
| sx={{ marginBottom: 1, ...style }} |
There was a problem hiding this comment.
css modules instead of sx
There was a problem hiding this comment.
I removed ...style since it wasn’t needed. It didn’t feel worth creating a separate file and compound selector just to apply a single marginBottom, so I kept it as sx={{ mb: 1 }}.
| sx={{ | ||
| marginTop: 16, | ||
| marginBottom: 32, | ||
| marginTop: 2, |
There was a problem hiding this comment.
css modules instead of sx
There was a problem hiding this comment.
This one’s a bit different. The styles prop is meant to override the default margins, and a few call sites rely on that. With the spread, the caller always wins. With a CSS module, the rules could tie and the override could depend on stylesheet load order.
| border-style: solid; | ||
| border-width: 2px; | ||
| border-radius: 100%; | ||
| font-size: 18px; |
There was a problem hiding this comment.
I'm skeptical that we should be customizing the font size of an MXUI component. @platypus801?
There was a problem hiding this comment.
The default font size from MUI is 12px and the 18px isn't arbitrary value I added. It was what the version before the css module conversion had, so I just preserved it.
| } | ||
|
|
||
| .paragraph:global(.MuiTypography-root) { | ||
| margin-bottom: var(--spacing-xlarge); |
There was a problem hiding this comment.
I'm not sure I follow the suggestion here.
| className={styles.badge} | ||
| color="error" | ||
| sx={(theme) => ({ | ||
| '& .MuiBadge-badge': { borderColor: theme.palette.common.white }, |
There was a problem hiding this comment.
css modules instead of sx
There was a problem hiding this comment.
We don’t have access to this theme color in a css module unless we hardcode it which I don’t think is the right approach.
| onClick={handleSubmit} | ||
| sx={{ | ||
| marginBottom: tokens.Spacing.XSmall, | ||
| marginBottom: 1, |
There was a problem hiding this comment.
css modules instead of sx
There was a problem hiding this comment.
I don’t think it’s worth creating a whole file and compound selector just to apply a single marginBottom.
| justifyContent: 'space-between', | ||
| padding: '0 0 16px 0', | ||
| marginTop: tokens.Spacing.XSmall, | ||
| marginTop: '1px', |
There was a problem hiding this comment.
Stacks for spacing. Do we really want a 1px margin? @platypus801
There was a problem hiding this comment.
Nice catch! This should be 8px.
There was a problem hiding this comment.
The wrapper here is RadioGroup that wraps two SelectionBox, I don't see how a Stack would apply here. I did fix the marginTop to be 8px though.
| return ( | ||
| <> | ||
| <Text bold={true} component="h2" sx={{ mb: 12 }} truncate={false} variant="H2"> | ||
| <Text bold={true} component="h2" sx={{ mb: 1.5 }} truncate={false} variant="H2"> |
There was a problem hiding this comment.
Stacks for spacing
There was a problem hiding this comment.
This is a single element with no other elements to stack with and it needs a custom margin.
| <div style={styles.container}> | ||
| <div style={styles.iconContainer}> | ||
| <AttentionFilled color={tokens.Color.NeutralWhite} size={24} /> | ||
| <Icon fill={true} name="error" size={24} sx={{ color: 'common.white' }} /> |
There was a problem hiding this comment.
css modules instead of sx
There was a problem hiding this comment.
We wouldn’t have access to the common.white theme color in a css module unless we hardcode it.
| data-test="verify-existing-member-header" | ||
| id="connect-select-institution" | ||
| sx={{ marginBottom: tokens.Spacing.Small }} | ||
| sx={{ marginBottom: 1.5 }} |
There was a problem hiding this comment.
I used Stack to position the elements and remove a bunch of styles but we still need custom margins for them.
| component="h3" | ||
| data-test="connected-institutions-text" | ||
| sx={{ marginBottom: tokens.Spacing.Tiny, fontWeight: 600 }} | ||
| sx={{ marginBottom: 0.5, fontWeight: 600 }} |
There was a problem hiding this comment.
Stacks for spacing. css modules instead of sx
It’s the default icon color from MXUI and per Jen’s suggestion, we don’t want to use custom colors. |
3 participants
Before(Kyper)
After(MXUI)