Fix Integration Button styles #50258
Conversation
If a user could list integrations, and they could NOT create integrations, AND integrations existed, this would cause the disabled 'enroll new integration' button to be smashed to the left of the header (this is due to how its wrapped in an optional Hover Tooltip). This PR just adds some space-between flex property to the header to ensure the button stays on the right side. It also moves the HoverTooltip position to the bottom because it looks better
avatus
added
backport-required
no-changelog
| @@ -79,7 +79,7 @@ export function Integrations() { | |||
| return ( | |||
| <> | |||
| <FeatureBox> | |||
| <FeatureHeader> | |||
| <FeatureHeader justifyContent="space-between"> | |||
There was a problem hiding this comment.
I'll take a closer look at this when I actually get around to reviewing the change. But just from a quick glance, I think it shouldn't be parent's responsibility to remember to set a certain flex attribute because a child might conditionally not render something. If possible, the child should make sure it's rendered correctly no matter the state. I'll see if that's doable here.
There was a problem hiding this comment.
The HoverTooltip has give me an issue with this in the past but this was back when I made it in v14 or whatever. Maybe it's easier now that the style system has been updated.
The issue is wanting to position the hover tooltip to its child but is wanting to render that child according to the tooltips parent. Last time i tried, this meant having to set something on the parent regardless. (This is also an issue when using it in a Table cell that you want to align right, for example). This could have all changed tho
I think merging this in before the next cut is preferable to reworking hovertooltip, and i can add a TODO to update the hovertooltip if you think that is sufficient.
There was a problem hiding this comment.
Sure, I'll approve now and we can work on this later.
There was a problem hiding this comment.
When I first skimmed through this, I was focused on "Why is it positioned correctly in one condition but not in the other", thinking that the callsite doesn't account for something. I looked into this today and what you did in this PR is the correct way to solve this.
It wasn't a problem with the callsite that uses IntegrationsAddButton, the problem was that IntegrationsAddButton used margin-left: auto to position itself to the far right of the screen. If possible, individual elements shouldn't decide how they're positioned in space, this should be preferably parent's decision. https://every-layout.dev/layouts/stack/#the-problem
So the usage of justify-content: space-between is correct here. We should get rid of margin-left: auto in IntegrationsAddButton.
public-teleport-github-review-bot
bot
removed request for
kiosion and
rudream
If a user could list integrations, and they could NOT create integrations, AND integrations existed, this would cause the disabled 'enroll new integration' button to be smashed to the left of the header (this is due to how its wrapped in an optional Hover Tooltip).
This PR just adds some space-between flex property to the header to ensure the button stays on the right side. It also moves the HoverTooltip position to the bottom because it looks better
old:
Updated
