Fix UI issues after introducing new button design system #43663
Conversation
bl-nero
force-pushed
the
bl-nero/buttons-2
branch
from
7cc43e9
to
270e199
Compare
bl-nero
force-pushed
the
bl-nero/buttons-2
branch
from
270e199
to
b7ef80e
Compare
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
bl-nero
added
the
no-changelog
| @@ -46,6 +46,7 @@ const StyledButtonLink = styled.a` | |||
| &:hover, | |||
| &:focus { | |||
| color: ${({ theme }) => theme.colors.buttons.link.hover}; | |||
| box-shadow: none; | |||
There was a problem hiding this comment.
I think it shouldn't have a background on hover either. The way I think about it is that ButtonLink is supposed to behave just like Link.
There was a problem hiding this comment.
You're probably right. I'll change it.
There was a problem hiding this comment.
I'm not going to change it right now, because it breaks a bit in places where link buttons are placed on a red background (the text turns illegible). As this coincides with your another comment, I'll fix it properly after I deal with the alert banners.
Also increase gap to increase visibility of individual groups
| <ButtonBorder | ||
| height="24px" | ||
| size="small" | ||
| setRef={e => (this.anchorEl = e)} | ||
| onClick={this.onOpen} | ||
| {...buttonProps} | ||
| > | ||
| {this.props.buttonText || 'OPTIONS'} | ||
| <ChevronDown ml={2} mr={-2} size="small" color="text.slightlyMuted" /> | ||
| {this.props.buttonText || 'Options'} |
There was a problem hiding this comment.
Just my opinion, but it's quite distracting for me how <Button fill="border"> loses its border on hover. 😅 But I suspect it was hard to represent the difference between all five states without this.
| @@ -63,14 +71,16 @@ export default function TrustedClusters() { | |||
| <FeatureHeader alignItems="center"> | |||
| <FeatureHeaderTitle>Trusted Clusters</FeatureHeaderTitle> | |||
| {hasClusters && ( | |||
| <ButtonPrimary | |||
There was a problem hiding this comment.
There's another place where ButtonPrimary is used in this file. Should it be removed as well?
There was a problem hiding this comment.
No, because this is an empty state with an actual CTA (the user has no trusted clusters, therefore this is the action that we recommend).
There was a problem hiding this comment.
Off topic, but I think disabled in the context of this component has been used as "A value that you cannot change because it's supposed to be hardcoded and always sent to the server", which should semantically be represented as readonly. disabled inputs are not sent to the server by the browser.
| <ButtonLink onClick={onRetryClicked}>Retry</ButtonLink> | ||
| <Button type="button" onClick={onRetryClicked}> | ||
| Retry | ||
| </Button> |
There was a problem hiding this comment.
This button didn't look quite right. I changed it a little bit, but it still doesn't look the best. idk what to do here, as we can't use fill any other than filled, because otherwise it blends with the red background. Also I wish the language server would suggest me "custom-defined" props first, because now if I place my cursor inside Button and ask my editor what props are available, it suggests me a bunch of aria props. ;f
@gzdunek Do you remember if there was a reason ButtonLink was used here instead of Button? This link doesn't have a href and we add onClick on it, which makes Button more suitable for it.
| Before | After |
|---|---|
There was a problem hiding this comment.
@gzdunek Do you remember if there was a reason ButtonLink was used here instead of Button? This link doesn't have a href and we add onClick on it, which makes Button more suitable for it.
Hmm it wasn't me who added that button 😅 but I agree, Buttons fits better here.
There was a problem hiding this comment.
Sorry, I just did a quick git blame and didn't dig further. 😏
There was a problem hiding this comment.
I'm going to leave it as is, since it's exactly how it looks now, and one of the next steps on my roadmap is to deal with the error banners, which will include modernizing their action buttons: https://www.figma.com/design/Gpjs9vjhzUKF1GDbeG9JGE/Application-Design-System?node-id=8890-5504&m=dev
| @@ -33,6 +33,7 @@ export type FileTransferRequest = { | |||
| sid: string; | |||
| requestID: string; | |||
| requester: string; | |||
| /** A list of accounts that approved this request. */ | |||
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This PR:
Caveat: since we are abandoning button capitalization, existing SSO buttons may look weird for those who have all-lowercase names of their SSO connectors. We may need to issue an announcement to our users about using the
displayNameconnector property to fix this.Closes #43175