Greater-London-Authority / ldn-viz-tools

https://greater-london-authority.github.io/ldn-viz-tools/
1 stars 0 forks source link

Input component does not grey out text when disabled #370

Closed PaulioRandall closed 1 month ago

PaulioRandall commented 3 months ago

Describe the bug

<Input disabled /> does not have greyed out text when disabled so it looks editable. However, the input label is greyed out.

To Reproduce

Write the following Svelte code:

import { Input } from '@ldn-viz/ui'

<Input name="meh" value="some text" disabled />

Expected behavior

The input text should be greyed out the same as the label text.

jamesscottbrown commented 3 months ago

The input box itself is greyed out: if the text inside the box was also greyed out it would be difficult to provide enough contrast between the text and background.

How the component currently looks in the disabled state: Screenshot 2024-06-26 at 12 10 18

PaulioRandall commented 3 months ago

The input box itself is greyed out: if the text inside the box was also greyed out it would be difficult to provide enough contrast between the text and background.

How the component currently looks in the disabled state: Screenshot 2024-06-26 at 12 10 18

In my case, low contrast is intended for disabled state. The content is irrelevant to the user (has no bearing on the final submission) if the input has been disabled. I don't delete the disabled content until they click 'continue' in case the user has a sudden change of heart.

In this case it looks like the inputs are still active: image

While this gives the impression the content is irrelevant but the user can see that they won't have to re-enter the information if they have a change of heart: image

Perhaps another prop? Or strikethrough applied to the text so it's clearly no longer part of the submission?

jamesscottbrown commented 3 months ago

In the storybook stories 1, it looks like in the dark theme the text color should already be changing when the input is disabled:

Enabled:

image

Disabled:

image

(this looks slightly odd as Storybook toggles the light/dark theme but doesn't change the background color behind the component)

ChrisKnightLDN commented 1 month ago

line 149 of input targets: $$restProps.disabled ? 'cursor-not-allowed' : '' in next I have updated this to $$restProps.disabled || disabled ? 'cursor-not-allowed text-color-input-label-disabled' : '', (being the correct color token)

This resolves the issue - but I'm not 100% why we target $$restProps.disabled rather than just disabled?

Screenshot 2024-08-05 at 15 09 08 Screenshot 2024-08-05 at 15 09 27

PaulioRandall commented 1 month ago

line 149 of input targets: $$restProps.disabled ? 'cursor-not-allowed' : '' in next I have updated this to $$restProps.disabled || disabled ? 'cursor-not-allowed text-color-input-label-disabled' : '', (being the correct color token)

This resolves the issue - but I'm not 100% why we target $$restProps.disabled rather than just disabled?

Screenshot 2024-08-05 at 15 09 08 Screenshot 2024-08-05 at 15 09 27

$$restProps are props passed to a component that were not explicitly defined so $$restProps.disabled won't exist if export let disabled prop exists. It should just be disabled ? 'cursor-not-allowed text-color-input-label-disabled'.

It's in a cloud of strings and ternary operators so I suspect it slipped past refactoring and review.

image

ChrisKnightLDN commented 1 month ago

Great, this is simplified on 'next'

Screenshot 2024-08-05 at 16 09 13

I can open a pr on the release branch that addresses this too.

ChrisKnightLDN commented 1 month ago

closing as done