feat(text-input): add read-only variant #3054

emyarod · 2019-06-13T14:35:23Z

This PR adds read-only support to the text input and textarea components alongside a mechanism to convert the inputs to controlled mode based on the onChange handler and value prop. This PR also reimplements character counter (which was previously reverted but depends on the controlled/uncontrolled separation). Since controlled/uncontrolled separation itself doesn’t provide any benefit from user’s perspective until character counter is introduced, it is being introduced alongside the character counter feature here

Changelog

New

mechanism to turn TextInput and TextArea to controlled components based on onChange handler and value prop
readOnly mode styles
character counter for TextInput and TextArea (ref: feat: text input and textarea character limit counter #2745 which was previously reverted)

Testing / Reviewing

Ensure the read-only text input and character counters behave as expected

packages/react/src/components/TextInput/TextInput.js

netlify · 2019-06-13T14:43:08Z

Deploy preview for the-carbon-components ready!

Built with commit 5276b58

https://deploy-preview-3054--the-carbon-components.netlify.com

netlify · 2019-06-13T14:51:33Z

Deploy preview for carbon-elements ready!

Built with commit 6faa37b

https://deploy-preview-3054--carbon-elements.netlify.com

netlify · 2019-06-13T15:01:53Z

Deploy preview for carbon-elements ready!

Built with commit 5276b58

https://deploy-preview-3054--carbon-elements.netlify.com

netlify · 2019-06-13T15:02:44Z

Deploy preview for carbon-components-react failed.

Built with commit 6faa37b

https://app.netlify.com/sites/carbon-components-react/deploys/5d025f43fcc7e3000743d743

netlify · 2019-06-13T15:26:20Z

Deploy preview for carbon-components-react ready!

Built with commit c33c472

https://deploy-preview-3054--carbon-components-react.netlify.com

shixiedesign

Looks great overall! A few small things:

clicking outside the tooltip should close the tooltip (vanilla only, react looks fine)
new tooltip should show above existing tooltip on screen, potentially not an issue if we can close the other tooltip easily
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.

emyarod · 2019-06-20T18:18:05Z

clicking outside the tooltip should close the tooltip (vanilla only, react looks fine)

@shixiedesign clicking outside of the tooltip in both vanilla and react closes the tooltip for me. can you provide more details about how to reproduce this issue?

elizabethsjudd · 2019-06-20T19:11:36Z

@emyarod Depending on what version of the tooltip you are using it could be related to this question i had: #3099 (comment)

emyarod · 2019-06-20T22:15:45Z

@elizabethsjudd this is using the WCAG 2.0 compliant tooltip. do you mean the behavior is inconsistent when a button is the trigger element?

elizabethsjudd · 2019-06-21T01:22:54Z

@emyarod when a button is used as the trigger for a tooltip, the user can "click" on the button and the tooltip will remain active until the user clicks somewhere else (the button is given focus which also causes the tooltip to display). You can scroll, resize, mouse over other elements, etc all while the tooltip remains open (this is what I believe is causing the scenario @shixiedesign is pointing out by having two tooltips open at once). The one thing I don't know is if this is accurate behavior because in actual offerings we'd expect a button to perform an action which would most likely cause the focus to change/move and therefore I believe the tooltip would hide. The question that I'm bringing up however is that I feel like both of these tooltips seem like they should be "definition tooltips" as they are either explaining the reason something is in a state, or it's just the extended version of the inputs value. My question in the other issue is that I don't feel like "definition tooltips" should be clickable at all unless they are on a button that is actually performing another action. In this use-case I don't feel that the tooltips should be on clickable triggers and the clicking is what is making them persistent. I've done a ton of research and implementations of various types of a11y tooltips so if you have any questions around this don't hesitate to ask.

emyarod · 2019-06-21T13:19:02Z

@elizabethsjudd that was intentional according to the initial component spec so that the user can select the content (although clicking on the read only input will also select all of the text). from what I understand, @shixiedesign is referring to clicking multiple times outside of the button before the tooltip closes, but I am unable to replicate that behavior (the tooltip is dismissed with one outside click for me). I think I need some clarification on what the bug report is referring to and some confirmation on the expected behavior

elizabethsjudd · 2019-06-21T13:28:25Z

@emyarod thanks for the clarification. Just out of curiosity, are you using the first tooltip that's on this page: https://www.carbondesignsystem.com/components/tooltip/code? At least on the site demo even after I open the tooltip if I try to select the text the tooltip closes (because it closes by clicking anywhere outside the trigger). WH found this when we were working with tooltips and thought this was odd behavior. Is that an update that's also coming with this PR?

emyarod · 2019-06-21T18:29:37Z

@elizabethsjudd, no I don't make any changes to the tooltip code in this PR so it seems you found a vanilla-only interactive tooltip bug. it looks like if I click on one of the interactive elements in the tooltip before trying to select the text the tooltip will not close (but only in the dev environment, not the Carbon website). probably need to examine the vanilla tooltip click handler. we can track this in a separate issue

elizabethsjudd · 2019-06-21T18:48:43Z

@emyarod totally agree that it is out of scope for this issue. Thank you for confirming!

elizabethsjudd · 2019-06-21T19:28:01Z

@emyarod I was just thinking about it. using aria-haspopup="true" would actually probably fix the "confusion" aspect that I was talking about earlier. Then the aria-controls applied dynamically should still allow JAWs users to toggle between the two when the the tooltip is open.

emyarod · 2019-06-24T13:03:02Z

@elizabethsjudd just to confirm, aria-haspopup="true" would be persistent and aria-controls would be added depending on if the tooltip content is inserted in the DOM right?

also is this related to #3100?

elizabethsjudd · 2019-06-24T15:17:27Z

@emyarod yes, and I would say that #3100 is related. I have a PR open for #3100 right now (see #3148). FYI though, #3148 only resolves AVT 1 issues. There are still issues with AVT 2 & 3 that I'm hoping to address next.

emyarod · 2019-06-25T21:29:19Z

@shixiedesign I think this is good for another review before I port this over to React but I may hold off until #3148 and associated PRs are approved and merged

shixiedesign

Looks good!

emyarod · 2019-09-16T14:54:03Z

depends on #3995 #4016

jnm2377 · 2019-09-17T15:11:31Z

@emyarod can you explain how this is being blocked by #3986 ? It seems like that PR needs a bigger discussion, so I'm wondering how we can get this PR moving along. This is blocking another stale PR.

emyarod · 2019-09-17T17:24:44Z

@jnm2377 I am currently exploring ways to work around it

joshblack · 2019-09-18T17:51:17Z

If we don't have a clear resolution in place, would be best to close this for now and revisit in our next components project 👍

joshblack · 2019-09-25T18:39:32Z

Closing this since the stale warning label was applied >7 days ago 👍

vpicone

I think we could use the :read-only pseudo-selector here rather than the attribute selector. Would likely only matter for folks overriding using the most "appropriate" selector.

Do we want to remove all the vanilla stuff from the PR?

vpicone · 2021-05-18T03:55:57Z

packages/react/src/components/TextInput/TextInput-story.js

@@ -80,7 +129,8 @@ storiesOf('TextInput', module)
    () => (
      <TextInput
        type={select('Form control type (type)', types, 'text')}
-        {...props.TextInputProps()}
+        {...props.textInput()}
+        invalid={props.textInput().readOnly ? false : props.textInput().invalid}


Nice work ignoring validation here, we definitely want to make sure that's the case in any readonly inputs

emyarod added type: question ❓ status: visual review 🔍 labels Jun 13, 2019

emyarod added this to the v10.4 milestone Jun 13, 2019

emyarod requested a review from shixiedesign June 13, 2019 14:35

jendowns reviewed Jun 13, 2019

View reviewed changes

packages/react/src/components/TextInput/TextInput.js Outdated Show resolved Hide resolved

emyarod force-pushed the 3018-read-only-text-input branch from 146d550 to 804fc13 Compare June 19, 2019 13:00

shixiedesign reviewed Jun 19, 2019

View reviewed changes

emyarod force-pushed the 3018-read-only-text-input branch 2 times, most recently from 9148838 to 8ac2852 Compare June 20, 2019 18:45

jnm2377 added the status: WIP 👩‍💻 label Jun 24, 2019

shixiedesign approved these changes Jun 26, 2019

View reviewed changes

emyarod added 5 commits September 16, 2019 09:52

refactor: consolidate value definitions

03c4201

refactor: invoke functions in render instead of IIFE

831ffc3

refactor: use updated tooltips

71f991b

fix(TooltipDefinition): support child nodes

b2172b9

refactor: use updated tooltips

1135b3c

emyarod force-pushed the 3018-read-only-text-input branch from 6557e1c to 1135b3c Compare September 16, 2019 14:54

emyarod requested a review from a team as a code owner September 16, 2019 14:54

ghost requested review from alisonjoseph and jnm2377 September 16, 2019 14:54

jnm2377 added the status: stale warning ⚠️ label Sep 16, 2019

Merge branch 'master' into 3018-read-only-text-input

aef8430

jnm2377 mentioned this pull request Sep 16, 2019

feat(TooltipDefinition): support different elements as tooltip trigger #3986

Closed

Merge branch 'master' into 3018-read-only-text-input

c33c472

jnm2377 removed their request for review September 17, 2019 15:07

jnm2377 removed the request for review from alisonjoseph September 17, 2019 15:13

Merge branch 'master' into 3018-read-only-text-input

5276b58

joshblack closed this Sep 25, 2019

emyarod mentioned this pull request Mar 25, 2020

Add optional character count to text input #5702

Closed

emyarod mentioned this pull request Oct 7, 2020

Read only text input #3018

Closed

vpicone reviewed May 10, 2021

View reviewed changes

vpicone reviewed May 18, 2021

View reviewed changes

janhassel mentioned this pull request May 31, 2021

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

Merged

emyarod deleted the 3018-read-only-text-input branch August 30, 2021 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(text-input): add read-only variant #3054

feat(text-input): add read-only variant #3054

emyarod commented Jun 13, 2019 •

edited

Loading

netlify bot commented Jun 13, 2019 •

edited

Loading

netlify bot commented Jun 13, 2019

netlify bot commented Jun 13, 2019 •

edited

Loading

netlify bot commented Jun 13, 2019

netlify bot commented Jun 13, 2019 •

edited

Loading

shixiedesign left a comment •

edited by emyarod

Loading

emyarod commented Jun 20, 2019 •

edited

Loading

elizabethsjudd commented Jun 20, 2019

emyarod commented Jun 20, 2019

elizabethsjudd commented Jun 21, 2019

emyarod commented Jun 21, 2019

elizabethsjudd commented Jun 21, 2019

emyarod commented Jun 21, 2019 •

edited

Loading

elizabethsjudd commented Jun 21, 2019

elizabethsjudd commented Jun 21, 2019

emyarod commented Jun 24, 2019 •

edited

Loading

elizabethsjudd commented Jun 24, 2019

emyarod commented Jun 25, 2019

shixiedesign left a comment

emyarod commented Sep 16, 2019 •

edited

Loading

jnm2377 commented Sep 17, 2019 •

edited

Loading

emyarod commented Sep 17, 2019

joshblack commented Sep 18, 2019

joshblack commented Sep 25, 2019

vpicone left a comment •

edited

Loading

vpicone May 18, 2021

feat(text-input): add read-only variant #3054

feat(text-input): add read-only variant #3054

Conversation

emyarod commented Jun 13, 2019 • edited Loading

Changelog

Testing / Reviewing

netlify bot commented Jun 13, 2019 • edited Loading

netlify bot commented Jun 13, 2019

netlify bot commented Jun 13, 2019 • edited Loading

netlify bot commented Jun 13, 2019

netlify bot commented Jun 13, 2019 • edited Loading

shixiedesign left a comment • edited by emyarod Loading

Choose a reason for hiding this comment

emyarod commented Jun 20, 2019 • edited Loading

elizabethsjudd commented Jun 20, 2019

emyarod commented Jun 20, 2019

elizabethsjudd commented Jun 21, 2019

emyarod commented Jun 21, 2019

elizabethsjudd commented Jun 21, 2019

emyarod commented Jun 21, 2019 • edited Loading

elizabethsjudd commented Jun 21, 2019

elizabethsjudd commented Jun 21, 2019

emyarod commented Jun 24, 2019 • edited Loading

elizabethsjudd commented Jun 24, 2019

emyarod commented Jun 25, 2019

shixiedesign left a comment

Choose a reason for hiding this comment

emyarod commented Sep 16, 2019 • edited Loading

jnm2377 commented Sep 17, 2019 • edited Loading

emyarod commented Sep 17, 2019

joshblack commented Sep 18, 2019

joshblack commented Sep 25, 2019

vpicone left a comment • edited Loading

Choose a reason for hiding this comment

vpicone May 18, 2021

Choose a reason for hiding this comment

emyarod commented Jun 13, 2019 •

edited

Loading

netlify bot commented Jun 13, 2019 •

edited

Loading

netlify bot commented Jun 13, 2019 •

edited

Loading

netlify bot commented Jun 13, 2019 •

edited

Loading

shixiedesign left a comment •

edited by emyarod

Loading

emyarod commented Jun 20, 2019 •

edited

Loading

emyarod commented Jun 21, 2019 •

edited

Loading

emyarod commented Jun 24, 2019 •

edited

Loading

emyarod commented Sep 16, 2019 •

edited

Loading

jnm2377 commented Sep 17, 2019 •

edited

Loading

vpicone left a comment •

edited

Loading