Skip to content

Conversation

@charliepark
Copy link
Contributor

Fixes #2255

With this PR, we're adding a ternary to the CSS display logic; if the button is disabled, we use the -disabled color; otherwise, we use the -secondary color.

In this screenshot, the top button is enabled; the bottom is disabled.
Screenshot 2024-05-21 at 5 22 03 PM

@vercel
Copy link

vercel bot commented May 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview May 22, 2024 4:21pm

<Button size="sm" className="shrink-0" {...props}>
<AddRoundel12Icon className="mr-2 text-accent-secondary" />
export const CreateButton = ({ children, disabled, ...props }: ButtonProps) => (
<Button size="sm" className="shrink-0" disabled={disabled} {...props}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we're now pulling the disabled prop out of the ...props spread on line 16, we need to add it in as an explicit prop in the Button component on 17.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another way would be to not pull it out and use props.disabled on line 19. either way is fine

@charliepark charliepark added this to the 9 milestone May 22, 2024
@charliepark charliepark self-assigned this May 22, 2024
await stopInstance(page)

await expect(page.getByRole('button', { name: 'Add network interface' })).toBeEnabled()
await page.click('role=button[name="Add network interface"]')
Copy link
Collaborator

@david-crespo david-crespo May 22, 2024

Choose a reason for hiding this comment

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

Line 45 is also the same selector, just done as a legacy string. Might as well pull out const addNicButton to use in all three spots.

@david-crespo
Copy link
Collaborator

I went to go remind myself why we have to set this color at all instead of relying on the default text color of the button — the problem was that the default text accent looks too bright next to the text when used to fill a nearly solid icon (Eugene has this in the design, we didn't make it up ourselves).

overridden default
Screenshot 2024-05-22 at 10 43 24 AM Screenshot 2024-05-22 at 10 43 37 AM

However, the disabled text color for the button is text-accent-disabled anyway, so we can get away with only overriding the color when it's not disabled.

diff --git a/app/ui/lib/CreateButton.tsx b/app/ui/lib/CreateButton.tsx
index 989efeef..7f6e9431 100644
--- a/app/ui/lib/CreateButton.tsx
+++ b/app/ui/lib/CreateButton.tsx
@@ -13,10 +13,12 @@ import { AddRoundel12Icon } from '@oxide/design-system/icons/react'
 
 import { Button, buttonStyle, type ButtonProps } from '~/ui/lib/Button'
 
-export const CreateButton = ({ children, disabled, ...props }: ButtonProps) => (
-  <Button size="sm" className="shrink-0" disabled={disabled} {...props}>
+export const CreateButton = ({ children, ...props }: ButtonProps) => (
+  <Button size="sm" className="shrink-0" {...props}>
     <AddRoundel12Icon
-      className={cn(disabled ? 'text-accent-disabled' : 'text-accent-secondary', 'mr-2')}
+      // dim the icon color from the default (text accent) because it looks a
+      // lot brighter than text, but default disabled color is fine
+      className={cn('mr-2', { 'text-accent-secondary': !props.disabled })}
     />
     {children}
   </Button>

@david-crespo david-crespo merged commit fc91ec1 into main May 22, 2024
@david-crespo david-crespo deleted the dim_plus_sign_on_disabled_button branch May 22, 2024 16:29
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request May 22, 2024
oxidecomputer/console@078d171...a228b75

* [a228b75b](oxidecomputer/console@a228b75b) bump omicron (only one tiny diff in validators)
* [fc91ec1e](oxidecomputer/console@fc91ec1e) oxidecomputer/console#2256
* [39b4491e](oxidecomputer/console@39b4491e) oxidecomputer/console#2230
* [e4e912ca](oxidecomputer/console@e4e912ca) oxidecomputer/console#2247
* [dcf09ec9](oxidecomputer/console@dcf09ec9) oxidecomputer/console#2217
* [c36b3d63](oxidecomputer/console@c36b3d63) oxidecomputer/console#2238
* [a8eb7745](oxidecomputer/console@a8eb7745) oxidecomputer/console#2251
* [9b20b7c9](oxidecomputer/console@9b20b7c9) oxidecomputer/console#2248
* [f20a5bcb](oxidecomputer/console@f20a5bcb) oxidecomputer/console#2245
* [b815dd8f](oxidecomputer/console@b815dd8f) oxidecomputer/console#2244
* [8c7b2946](oxidecomputer/console@8c7b2946) add node_modules to eslint ignore patterns
* [90e78dbb](oxidecomputer/console@90e78dbb) oxidecomputer/console#2237
* [b603d2dd](oxidecomputer/console@b603d2dd) oxidecomputer/console#2242
* [bfce37c7](oxidecomputer/console@bfce37c7) upgrade @oxide/openapi-gen-ts to 0.2.2
* [efceb17d](oxidecomputer/console@efceb17d) oxidecomputer/console#2236
* [1aa46459](oxidecomputer/console@1aa46459) oxidecomputer/console#2235
* [b400ae78](oxidecomputer/console@b400ae78) oxidecomputer/console#2225
* [7bb3bbf7](oxidecomputer/console@7bb3bbf7) oxidecomputer/console#2229
* [c56a9ec5](oxidecomputer/console@c56a9ec5) oxidecomputer/console#2228
* [cd9d1f99](oxidecomputer/console@cd9d1f99) oxidecomputer/console#2227
* [ee269bd9](oxidecomputer/console@ee269bd9) oxidecomputer/console#2223
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request May 22, 2024
Highlights: soft image validation, logout button on error pages to help
deal with auth-related errors.

oxidecomputer/console@078d171...a228b75

* [a228b75b](oxidecomputer/console@a228b75b)
bump omicron (only one tiny diff in validators)
* [fc91ec1e](oxidecomputer/console@fc91ec1e)
oxidecomputer/console#2256
* [39b4491e](oxidecomputer/console@39b4491e)
oxidecomputer/console#2230
* [e4e912ca](oxidecomputer/console@e4e912ca)
oxidecomputer/console#2247
* [dcf09ec9](oxidecomputer/console@dcf09ec9)
oxidecomputer/console#2217
* [c36b3d63](oxidecomputer/console@c36b3d63)
oxidecomputer/console#2238
* [a8eb7745](oxidecomputer/console@a8eb7745)
oxidecomputer/console#2251
* [9b20b7c9](oxidecomputer/console@9b20b7c9)
oxidecomputer/console#2248
* [f20a5bcb](oxidecomputer/console@f20a5bcb)
oxidecomputer/console#2245
* [b815dd8f](oxidecomputer/console@b815dd8f)
oxidecomputer/console#2244
* [8c7b2946](oxidecomputer/console@8c7b2946)
add node_modules to eslint ignore patterns
* [90e78dbb](oxidecomputer/console@90e78dbb)
oxidecomputer/console#2237
* [b603d2dd](oxidecomputer/console@b603d2dd)
oxidecomputer/console#2242
* [bfce37c7](oxidecomputer/console@bfce37c7)
upgrade @oxide/openapi-gen-ts to 0.2.2
* [efceb17d](oxidecomputer/console@efceb17d)
oxidecomputer/console#2236
* [1aa46459](oxidecomputer/console@1aa46459)
oxidecomputer/console#2235
* [b400ae78](oxidecomputer/console@b400ae78)
oxidecomputer/console#2225
* [7bb3bbf7](oxidecomputer/console@7bb3bbf7)
oxidecomputer/console#2229
* [c56a9ec5](oxidecomputer/console@c56a9ec5)
oxidecomputer/console#2228
* [cd9d1f99](oxidecomputer/console@cd9d1f99)
oxidecomputer/console#2227
* [ee269bd9](oxidecomputer/console@ee269bd9)
oxidecomputer/console#2223
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plus in create button needs to fade when disabled

3 participants