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

Read only number input #12384

Merged
merged 12 commits into from
Nov 14, 2022
Merged

Conversation

lee-chase
Copy link
Member

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

Contributes to #2177

Adjusts the readOnly NumberInput based on #2177.

Closes #12613

Changelog

Changed

Testing / Reviewing

Reviewed the result in Storybook, there is no readOnly test for text input.

@lee-chase lee-chase requested a review from a team as a code owner October 25, 2022 11:01
@netlify
Copy link

netlify bot commented Oct 25, 2022

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7e6164c
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6372650bc50cdf0009e2587c
😎 Deploy Preview https://deploy-preview-12384--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 25, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 7e6164c
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6372650b20ac14000829fa9a
😎 Deploy Preview https://deploy-preview-12384--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.

LGTM pending one question to design about the token

@tay1orjones tay1orjones requested review from a team and laurenmrice and removed request for a team October 25, 2022 12:52
@lee-chase lee-chase force-pushed the readOnlyNumberInput branch from 74294d2 to 382d219 Compare November 4, 2022 15:44
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looking good! Just a couple comments:

-/+ controls
On hover:

  • The cursor should be the Arrow icon, not the pointer icon.
  • The browser-based tooltips for decrement/increment should not appear.
  • When hovering over the controls area there is a bug with a slight change in color that appears on the divider rule.

@lee-chase
Copy link
Member Author

lee-chase commented Nov 8, 2022

Looking good! Just a couple comments:

-/+ controls On hover:

  • The cursor should be the Arrow icon, not the pointer icon.
  • The browser-based tooltips for decrement/increment should not appear.
  • When hovering over the controls area there is a bug with a slight change in color that appears on the divider rule.

@laurenmrice the cursor is arrow on button hover, the tooltip (Chrome 107, Safari 16, Firefox 106) does not display. Perhaps the build was out of date.

Fixed the divider color.

Kapture 2022-11-08 at 13 48 27

@lee-chase lee-chase requested review from laurenmrice and removed request for aledavila November 8, 2022 13:50
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks good!

@kodiakhq kodiakhq bot merged commit 88243d4 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]: NumberInput implementation
4 participants