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: readonly checkbox #12398

Merged
merged 10 commits into from
Nov 14, 2022
Merged

Conversation

lee-chase
Copy link
Member

@lee-chase lee-chase commented Oct 26, 2022

Contributes to #2177
Closes #12243

Adds the read-only feature to the Checkbox component

Changelog

Added

  • Adds readOnly property to Checkbox and functionality
  • Adds test for read-only checkbox
  • Adds styling for read-only checkbox (checked and unchecked).

Testing / Reviewing

Reviewed the result in Storybook and a added read-only test.

@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit f9826ab
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6372acb2acce9f0009ebb1d4
😎 Deploy Preview https://deploy-preview-12398--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit f9826ab
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6372acb2c88586000828e86d
😎 Deploy Preview https://deploy-preview-12398--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

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.

Overall looks good, just a few comments

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

The styles look good but there are some interaction problems.

I'm still able to click on the check-box, has a cursor change to a hand and focus border. My assumption is those things shouldn't be possible because it is giving the illusion that its interactive. (Interactions aren't explicitly stated in the spec though we might need to circle back with Devin and team).

Update: Oh is that the onClick thing you're talking about with @tay1orjones? At a minimum, can we not have the cursor change, can it just remain an arrow or switch to the not-allowed cursor?

Oct-28-2022 14-03-00

@lee-chase
Copy link
Member Author

lee-chase commented Nov 1, 2022

@aagonzales @tay1orjones was referring to the internals of the component I believe.

With regards to typical behaviour readonly is only applied to text input fields. The only distinction from disabled is that it is focusable and will be submitted with forms.

With regards to the cursor I'm not sure if https://github.com/quarryboy or Mike Eaker would care to comment?

The default behaviour (for input fields) can be seen in the link below. The cursor stays as it would normally. That is to say it displays the text cursor in most cases, the deafault pointer for date and the se-resize at the corner of a textarea.

As it is focusable and the cursor ordinarily stays the same for input fields, it suggests to me it should do so here.

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly

@aagonzales
Copy link
Member

@lee-chase I talked with Mike and Devin over Slack. Here are the confirmed expected interactive behaviors. So the changes needed are:

  • Hovering over the checkbox icon there should be no cursor change (remains as an arrow)
  • Hovering over the checkbox labels the cursor should change to a type cursor (not a hand)

image

@lee-chase
Copy link
Member Author

@tw15egan merge conflict fixed

@lee-chase
Copy link
Member Author

@aagonzales addressed cursor issue

@lee-chase lee-chase requested review from aagonzales and removed request for alisonjoseph and tw15egan November 7, 2022 11:00
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Visually looks good, matches spec 👍

@kodiakhq kodiakhq bot merged commit baa7fd5 into carbon-design-system:main Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Read-only Inputs: Checkbox Implementation
5 participants