Clean up discover alerts #51842
Clean up discover alerts #51842
Conversation
zmb3
added
no-changelog
Discover was using a custom HintBox component instead of the in-page alerts that are part of our design system. As a result, some items rendered without enough contrast. Use the new alert component to ensure consistency across the UI.
zmb3
force-pushed
the
zmb3/discover-dialogs
branch
from
4befbfa to
ffd17ee
Compare
web/packages/teleport/src/Discover/Kubernetes/SelfHosted/HelmChart/HelmChart.tsx
Outdated
Show resolved
Hide resolved
web/packages/teleport/src/Discover/Kubernetes/EnrollEKSCluster/AgentWaitingDialog.tsx
Outdated
Show resolved
Hide resolved
web/packages/teleport/src/Discover/Database/DeployService/ManualDeploy/ManualDeploy.tsx
Outdated
Show resolved
Hide resolved
web/packages/teleport/src/Discover/Database/DeployService/ManualDeploy/ManualDeploy.tsx
Outdated
Show resolved
Hide resolved
| @@ -302,6 +302,7 @@ const IconContainer = styled.div<{ kind: AlertKind }>` | |||
| border-radius: 50%; | |||
| line-height: 0; | |||
| margin-right: ${p => p.theme.space[3]}px; | |||
| align-self: flex-start; | |||
There was a problem hiding this comment.
Top-align the icon in alerts with large amounts of vertical content.
I wanted to do the same thing, but I don't think you can just change IconContainer like this, as it'll affect all alerts across the whole app. Those that are supposed to be on a single line are going to look off (e.g. https://localhost:9002/?path=/story/design-alerts--actionable).
This needs to be configurable on a per-alert basis.
There was a problem hiding this comment.
My solution keeps the original placement of the icon for other alerts.
| Zac's | Rafał's |
|---|---|
 |
 |
I added alignItems which allows you to align both the icon and the action buttons to the top. I'm going to need it in VNet diagnostics anyway. Would using it work for you in Discover?
I also added a story with controls for Alert so that it's easier to test different variants.
There was a problem hiding this comment.
Oops, I didn't know you talked with Bartosz about this. I still think it might be best to change only the Discover alerts for now?
There was a problem hiding this comment.
Your solution seems fine, but I am confused at the state of things now that you've pushed to my branch.
Are we good to go now on this issue or are more changes required?
There was a problem hiding this comment.
You'd need to add alignItems="flex-start" to relevant alerts in Discover where the icon should be aligned to the top. I suppose every warning that used to use HintBox should now be <Warning alignItems="flex-start">.
zmb3
force-pushed
the
zmb3/discover-dialogs
branch
from
f153423 to
87f8c0f
Compare
public-teleport-github-review-bot
bot
removed the request for review
from rudream
| // By default, <a> within an Alert has white color which has bad contrast on certain kinds of | ||
| // alerts, like Warning for example. This is an attempt to at least make them visible using the | ||
| // same color as the text. | ||
| & ${Link}, & a { | ||
| color: inherit; |
There was a problem hiding this comment.
Ugh, I can't do this without risking breaking someone's UI if we ever used something like <ButtonPrimary as="a"> within an alert.
There was a problem hiding this comment.
Fixed by specifically checking against <Button>.
There was a problem hiding this comment.
I didn't notice that in OuterContainer we hardcode links to theme.colors.light, which is something last touched 6 years ago. I just changed it to color: inherit instead.
Relevant stories:
- https://localhost:9002/?path=/story/teleport-discover-connectmycomputer-setupconnect--hint-timeout&globals=theme:Light+Theme
- https://localhost:9002/?path=/story/teleporte-accesslists-view--viewing-as-admin&globals=theme:Light+Theme
- Click "Delete" in the top right, the modal has a link in an alert and uses
linkColorprop of<Alert>to change the color of the link.
- Click "Delete" in the top right, the modal has a link in an alert and uses
I couldn't use the same color as Link, as it looks bad with the BBLP theme.
There was a problem hiding this comment.
@kimlisa Could you double check me here?
…which are rendered as `<a>`, e.g. `<ButtonPrimary as="a">`.
| Link.displayName = 'Link'; | ||
|
|
||
| const StyledButtonLink = styled.a.attrs({ | ||
| const Link = styled.a.attrs({ |
There was a problem hiding this comment.
This change can stay, <StyledButtonLink> was unnecessarily wrapped in Link instead of being exported directly.
Discover was using a custom HintBox component instead of the in-page alerts that are part of our design system. As a result, some items rendered without enough contrast. Use the new alert component to ensure consistency across the UI.
Example after/before:
Other changes:
-character to emulate a bullet.Before:
After:
