flex: cleanup Flex* custom styling in favor of Flex primitive #93809

JonasBa · 2025-06-18T13:56:55Z

Just remove a couple custom Flex* classes in favor of the Flex primitive

natemoo-re

Generally in favor of this!

Not a blocker, but still not sure where I land on the styled(Flex) pattern. Intuitively feels like we shouldn't need to create an override to define padding / overflow behavior?

natemoo-re · 2025-06-18T15:05:08Z

static/app/components/codeSnippet.tsx

@@ -163,12 +164,12 @@ export function CodeSnippet({
                </Tab>
              ))}
            </TabsWrapper>
-            <FlexSpacer />
+            <Flex.Item grow={1} />


Wonder if we should have <Flex.Spacer /> as a shorthand here? Seems easier to remember than this.

Yeah, that could also work. I don't have a strong opinion on it tbh, it seems like an edge case we can iron out later

natemoo-re · 2025-06-18T15:06:55Z

static/app/utils/discover/styles.tsx

@@ -52,9 +52,6 @@ export const BarContainer = styled('div')`
 `;

 export const FlexContainer = styled('div')`
-  display: flex;
-  align-items: center;
-  justify-content: flex-end;
  width: 100%;
 `;


Missing a replacement here?

Ah no, I actually want to revert this one, and possibly just rename the component given that it's exported

scttcper · 2025-06-18T15:50:41Z

static/app/components/group/times.tsx

@@ -42,12 +43,8 @@ const Container = styled('div')`
  min-width: 0; /* flex-hack for overflow-ellipsised children */
 `;

-const FlexWrapper = styled('div')`
+const FlexWrapper = styled(Flex)`


personally think this is worse

also it changes how it displays since overflowEllipsis is overriding display flex

I don't disagree, but I think it suffers from the same problem as inline styles (which are somewhat growing in popularity across the codebase).

I will try and modify these styled usages to use TextOverflow (at least in this case, it should help)

I've reverted the change locally, to make sure I don't break anything, and I'm either not testing this correctly, or it doesn't even work in it's current state.

CleanShot.2025-06-18.at.14.02.09.mp4

scttcper · 2025-06-18T15:51:21Z

static/app/views/alerts/list/incidents/row.tsx

@@ -99,10 +100,8 @@ const ProjectBadge = styled(IdBadge)`
  flex-shrink: 0;
 `;

-const FlexCenter = styled('div')`
+const FlexCenter = styled(Flex)`


this doesn't work

Same as the comment above, I will change it to use TextOverflow

Unfortunately, I don't think this one works either. I've reverted the change and wrote a story to test (using PanelTable as the wrapper to mimic the usage), and this is what I see

scttcper · 2025-06-18T15:51:55Z

static/app/views/performance/newTraceDetails/traceWaterfall.tsx

-  display: flex;
+const FulllHeightFlex = styled(Flex)`
  flex-grow: 1;


i think this is also worse

I think it can actually be avoided by moving the container to the parent component, I will try to do that.

scttcper

avoid styled(Flex) at all costs

scttcper · 2025-06-18T15:54:30Z

static/gsAdmin/views/instanceLevelOAuth/instanceLevelOAuthDetails.tsx

-  display: flex;
+const FullWidthFlex = styled(Flex)`
  width: 100%;


probably actually a flex-grow 1

We don't even need it, just flex with align right looks identical

scttcper · 2025-06-18T15:54:56Z

static/app/views/settings/organizationIntegrations/detailedView/integrationLayout.tsx

@@ -244,7 +244,7 @@ function InformationCard({
  return (
    <Fragment>
      <Flex align="center">
-        <FlexContainer>
+        <Flex.Item grow={1}>


What is a flex item?

currently, just a convenient way of setting the flex property on the child.

JonasBa · 2025-06-18T16:33:25Z

avoid styled(Flex) at all costs

We can't unfortunately do that. The fact that we are using styled components won't give us the isolation we need.

scttcper · 2025-06-18T16:36:47Z

i can lint against it. It is too hard to understand what the styles are. Even in this pr we're overriding display: flex in the wrong way

JonasBa · 2025-06-18T16:37:41Z

Generally in favor of this!

Not a blocker, but still not sure where I land on the styled(Flex) pattern. Intuitively feels like we shouldn't need to create an override to define padding / overflow behavior?

I agree. I think that calls for a better set of primitives or a more complete API - this change mostly surfaces that problem.

JonasBa · 2025-06-18T16:45:48Z

i can lint against it. It is too hard to understand what the styles are. Even in this pr we're overriding display: flex in the wrong way

Linters can be disabled, and I wouldn't count on them as the ultimate solution. Ultimately, what we want it to give people the least amount of reason to even use styles for anything. We are pretty far from that, but this is a start.

You are right about pointing out the problem about overriding display, I missed that, however the root cause is still in the fact that we are restyling things and that overflow ellipsis ends up overriding display, which is imo not what it should be doing. I would bet you that there are actually cases in the codebase where that bug already exists today.

JonasBa · 2025-06-18T16:48:45Z

@scttcper it really didn't take that long to find one either, and I'm pretty sure we have more

sentry/static/app/components/deprecatedSmartSearchBar/searchDropdown.tsx

Lines 584 to 595 in 480e641

    
             display: flex; 
        
             flex-grow: 1; 
        
             flex-shrink: ${p => (p.hasSingleField ? '1' : '0')}; 
        
             max-width: ${p => (p.hasSingleField ? '100%' : 'min(280px, 50%)')}; 
        
             color: ${p => p.theme.textColor}; 
        
             font-weight: ${p => p.theme.fontWeightNormal}; 
        
             font-size: ${p => p.theme.fontSizeMedium}; 
        
             margin: 0; 
        
             line-height: ${p => p.theme.text.lineHeightHeading}; 
        
             ${p => p.theme.overflowEllipsis};

scttcper · 2025-06-18T16:53:57Z

the problem i have is that styles now come from 3 places

Flex has styles, the props are declaring styles <Flex column> and you can override them in styled(Flex). This is crazy when we just want a div with flex.

scttcper · 2025-06-18T16:55:06Z

still in favor of adopting a subset of tailwind display styles. Would reduce runtime overhead at least

JonasBa · 2025-06-18T17:08:50Z

the problem i have is that styles now come from 3 places

Flex has styles, the props are declaring styles <Flex column> and you can override them in styled(Flex). This is crazy when we just want a div with flex.

I agree, and I want us to move away from both inline styled and styled components. I will see if I can actually move entirely away of the styled flex in this PR, I think it should be doable.

For the question of "just wanting div with flex", there is usually more behind that, like you most probably want something else with that display flex too, maybe direction='column', or something, which is where other higher level primitives can come in, things like Stack or List, those have better semantic meaning, but also allow you to tweak the subset like gap, as opposed to having to set display=column on everything. We don't provide those, but we probably should.

scttcper · 2025-06-18T17:40:11Z

I'll never use stack or list because I have styled('div') and won't remember they exist. just like nobody knows Flex.Item exists

JonasBa · 2025-06-18T17:48:35Z

I'll never use stack or list because I have styled('div') and won't remember they exist. just like nobody knows Flex.Item exists

If we never add documentation or guidelines for any of this, then neither will anyone else for that matter, but that's actually something we want to change.

The assumption is that if the primitives are good, people won't need to use styled. Now, I don't know if we will actually get there in practice, I think it's going to be hard, however we should at least be able to get to a point where styled is used much less frequently than it is today.

flex: cleanup Flex* classes

f647847

JonasBa requested review from a team as code owners June 18, 2025 13:56

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 18, 2025

vercel bot deployed to Preview June 18, 2025 13:58 View deployment

natemoo-re approved these changes Jun 18, 2025

View reviewed changes

scttcper reviewed Jun 18, 2025

View reviewed changes

scttcper requested changes Jun 18, 2025

View reviewed changes

scttcper reviewed Jun 18, 2025

View reviewed changes

JonasBa added 2 commits June 18, 2025 13:53

ref: revert flex change in tracedetailsheader

2345d0a

ref: use TextOverflow in group times

fadc8c7

JonasBa added 2 commits June 18, 2025 14:10

revert discover flex change

3788afb

revert groupEventAttachments flex change

a820348

vercel bot deployed to Preview June 18, 2025 18:42 View deployment

alternative approach for AlertListRow

7f64ee0

vercel bot deployed to Preview June 18, 2025 19:08 View deployment

JonasBa added 4 commits June 18, 2025 15:19

revert AlertListRow until we have min-width on flex

11b9bf5

fix flex in waterfall

5d00d6c

fix flex in gsadmin

fbf581b

remove ephemeral stories

5707e3e

vercel bot deployed to Preview June 18, 2025 19:33 View deployment

revert samplerateinput flex change

54c5f8d

vercel bot deployed to Preview June 18, 2025 20:36 View deployment

scttcper approved these changes Jun 18, 2025

View reviewed changes

Uh oh!

flex: cleanup Flex* custom styling in favor of Flex primitive #93809

Are you sure you want to change the base?

flex: cleanup Flex* custom styling in favor of Flex primitive #93809

Uh oh!

Conversation

JonasBa commented Jun 18, 2025

Uh oh!

natemoo-re left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scttcper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonasBa commented Jun 18, 2025

Uh oh!

scttcper commented Jun 18, 2025

Uh oh!

JonasBa commented Jun 18, 2025

Uh oh!

JonasBa commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonasBa commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scttcper commented Jun 18, 2025

Uh oh!

scttcper commented Jun 18, 2025

Uh oh!

JonasBa commented Jun 18, 2025

Uh oh!

scttcper commented Jun 18, 2025

Uh oh!

JonasBa commented Jun 18, 2025

Uh oh!

Uh oh!

JonasBa commented Jun 18, 2025 •

edited

Loading

JonasBa commented Jun 18, 2025 •

edited

Loading