core: remove a couple of styled(flex) calls #102930
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| 'wrap', | ||
| ]); | ||
|
|
||
| type PlaceItems = 'start' | 'end' | 'center' | 'baseline' | 'stretch'; |
There was a problem hiding this comment.
Bug: Prop Forwarding: Missing Omit for placeItems Prop
The placeItems prop was added to the FlexLayoutProps interface and used in the styled component, but wasn't added to the omitFlexProps Set. This causes the prop to be forwarded to the DOM element, which will trigger React warnings about unknown DOM attributes.
JonasBa
force-pushed
the
jb/styled/core-cleanup
branch
from
53be74c to
670ac22
Compare
| /> | ||
| </Section> | ||
| <ButtonWrapper justify="between"> | ||
| <ButtonWrapper justify="between" borderTop="primary" padding="xl"> |
There was a problem hiding this comment.
Bug: Padding escalation alters component's visual layout
The padding prop changed from lg (12px) to xl (16px), altering the component's visual appearance. The original styled component used theme.space.lg but the new code applies padding="xl" to ButtonWrapper, increasing padding from 12px to 16px and affecting the layout.
| return ( | ||
| <Flex | ||
| gap="xs" | ||
| display="flex" |
| gap: ${p => p.theme.space.sm}; | ||
| `; | ||
| function Step(props: FlexProps) { | ||
| return <Stack gap="sm" {...props} />; |
| border-top: 1px solid ${p => p.theme.border}; | ||
| padding: ${p => p.theme.space.lg}; | ||
| margin: -${p => p.theme.space.lg}; | ||
| margin: -${p => p.theme.space.xl}; |
There was a problem hiding this comment.
I'm in favor of keeping the negative-margin and padding statements together, so it's easier to see that they're related
There was a problem hiding this comment.
Are they? I'm struggling to understand why we need negative margins
| const ButtonWrapper = styled(Flex)` | ||
| border-top: 1px solid ${p => p.theme.border}; | ||
| padding: ${space(2)}; | ||
| margin: -${space(2)}; |
There was a problem hiding this comment.
ditto negative-margin and padding staying together.
There was a problem hiding this comment.
Seeing this again, maybe the technique deserves a built-in mechanism to make it more declarative?
For example: instead of styled(Flex) and <ButtonWrapper padding="xl"> we could try <Flex padding="xl" margin="-padding">.
That, or something else. My first thought was a new display type: I've heard this technique called full-bleed in the past, but idk if i like that name now <Flex display="full-bleed">.
There was a problem hiding this comment.
I don't want people to use margins at all, and especially not negative ones. I need to understand the use case here, because in my 4y at Sentry, I've never had to use negative margins
There was a problem hiding this comment.
i just recognize the technique as a way to to break out of the current container and have the background fill up what would normally be padding provided by the container.
I actually used this technique to make rows on the replay index page have larger click-targets. Normally <SimpleTableCell> has it's own built-in padding, and clicking into that padded area wouldn't register on links or button or whatever. So we use negative margin that matches the TableCell padding, then add the same padding again onto the link to compensate.
equiv to:
<td style={{padding: '16px'}}>
<a href="/replay/123" style={{margin: '-16px', padding: '16px'}}>
Replay Id: {replay.id}
</a>
</td>
To do it another way we'd need to do some other contortions to make it work. This example btw maybe dropping the padding on the td would work, but then the engineer would be more in charge of putting the correct <td withPadding={boolean}> in place instead of the table doing that automatically; to that could be a tradeoff.
I don't like <p style={{margin-bottom: '1em'}}> at all but maybe constraining margin to only&always be the inverse of padding makes this css technique work with the dom/aria parent-child hierarchy we have, without creating the layout/sibling dependencies that we don't like.
ryan953
left a comment
ryan953
left a comment
There was a problem hiding this comment.
Looked at all the files, all changes look mechanical & correct
Remove redundant calls to styled that probably predate many of the API changes like margins and borders.