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.

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

feat(text-input): add readonly variant #8806

Merged
merged 7 commits into from
Jun 17, 2021

Conversation

janhassel
Copy link
Member

Ref #2177, #3018, #3054

Adds a readonly variant to the TextInput component.

Intentional differences from the original design spec:

  • No tooltip on the EditOff16 icon
    • Looking at the planned changes of the tooltip component in v11, adding full support for that at this time seems inefficient. It would require the addition of multiple props that aren't really in the scope of the text input but rather of the tooltip (tooltipAlignment, tooltipPosition) and might introduce accessiblity issues that are to be addressed with the tooltip changes in v11.
    • In the meantime, teams can still inform their users about the reason why a field is read-only with a couple of options:
      • Use the helper text as explaination
      • Include a tooltip in the form label (quick poc with disabled field: https://jfckr.csb.app/)
  • No tooltip on the entire input to show overflow text
    • Same as above
    • Users have the ability to "scroll" inside the field with both their mouse or their keyboard (arrow keys) to reveal overflowing text (native browser behaviour)

Changelog

New

  • props.readonly on TextInput

Testing / Reviewing

  • Ensure readonly attribute is set to <input> accordingly
  • Ensure no visual regressions appear with any combination of invalid, warn, readonly on both the TextInput and the PasswordInput component
  • Ensure that no error states are shown when a field is marked as readonly

Side note: this PR also adds a helper useNormalizedInputProps which aims to reduce some of the many conditional renderings that were introduced over time (e.g. {!invalid && !warn && !isFluid && !inline && helper}) by outsourcing them and providing a single entry for the icons and validation messages.
The intent is to add readonly support to other inputs as well (like NumberInput). This utility should help there as well and streamline the inputs. Let me know what you think!

@janhassel janhassel requested review from a team as code owners May 31, 2021 15:47
@netlify
Copy link

netlify bot commented May 31, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: f2d8a50

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60b5050a3cf4fd00075c164e

😎 Browse the preview: https://deploy-preview-8806--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 31, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 64b2ccb

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60cb6c6b136f2800080e202c

😎 Browse the preview: https://deploy-preview-8806--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 31, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: f2d8a50

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60b5050ab60b2400078d33c8

😎 Browse the preview: https://deploy-preview-8806--carbon-components-react.netlify.app/iframe

@netlify
Copy link

netlify bot commented May 31, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 64b2ccb

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60cb6c6bfe17a100081396d4

😎 Browse the preview: https://deploy-preview-8806--carbon-components-react.netlify.app/iframe

Copy link
Member

@sstrubberg sstrubberg left a comment

Choose a reason for hiding this comment

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

Read-only appears to take precedence over warn and invalid. Was that the intended outcome?

Also, the stuff below.

packages/react/src/components/TextInput/TextInput.js Outdated Show resolved Hide resolved
@janhassel
Copy link
Member Author

@sstrubberg Yes, when readonly is passed, both invalid and warn should be ignored. I based this on @shixiedesign's comment from two years ago:

Is it possible to disable "error state" on read-only? This situ shouldn't arise. It doesn't give user a way forward since field cannot be edited.

@sstrubberg sstrubberg self-requested a review June 2, 2021 14:58
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

When disabled, I think the icon should probably be modified to match text color?

image

Also how should we treat it with the light prop? Selecting between the readonly and light knobs there is no visual difference other than the icon. @carbon-design-system/design

light-readonly-toggle.mov


const { prefix } = settings;

export function useNormalizedInputProps({
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good point of abstraction. Could you add a jsdoc-type comment for it so we have some context on what this hook accomplishes, when it should be used, expected param types, etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed an update adding some guidance aorund it. Let me know what you think of it!

@janhassel
Copy link
Member Author

@tay1orjones Actually, now that you bring it up: readonly should probably take precedence over disabled / block disabled from being applied. Or can you think of a use case where a disabled read-only field would make sense?

Copy link
Contributor

@johnbister johnbister left a comment

Choose a reason for hiding this comment

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

If read only takes precedence over disabled being applied, it looks fine visually. But if you think of a use case where a disabled read-only would be used, I would make the icon the same color as the disabled text like Taylor said. Let me know what you decide and I’ll approve.

@johnbister
Copy link
Contributor

johnbister commented Jun 14, 2021

readonly taking precedence over disabled looks good for textinput!

@janhassel I don't see the readonly variant for the passwordinput. Is this PR still adding the readonly variant to both textinput and passwordinput? I apologize if I'm just missing something.

@janhassel
Copy link
Member Author

@johnbister My plan would be to add support for readonly step by step to more components. I think building out the ground work in this one and then apply it to others can help keep the PRs manageable.

@johnbister johnbister self-requested a review June 14, 2021 15:29
Copy link
Contributor

@johnbister johnbister left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@johnbister johnbister left a comment

Choose a reason for hiding this comment

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

lgtm!

@janhassel
Copy link
Member Author

@tay1orjones Regarding the light prop question: Afaik the design intent is to not have any background when the input is readonly, so I set it to transparent. Therefore there is no visual differenation between light and regular.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up @janhassel , sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants