Skip to content
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.

Sign up for GitHub

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

Fix Integration Button styles #50258

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Fix Integration Button styles #50258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion web/packages/teleport/src/Integrations/Integrations.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export function Integrations() {
return (
<>
<FeatureBox>
<FeatureHeader>
<FeatureHeader justifyContent="space-between">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll approve now and we can work on this later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<FeatureHeaderTitle>Integrations</FeatureHeaderTitle>
<IntegrationsAddButton
requiredPermissions={[
Expand Down
1 change: 1 addition & 0 deletions web/packages/teleport/src/Integrations/IntegrationsAddButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export function IntegrationsAddButton({

return (
<HoverTooltip
position="bottom"
tipContent={
canCreateIntegrations ? null : (
<MissingPermissionsTooltip
Expand Down
Loading