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

bug(checkbox): added missing readonly prop value #3859

Merged
merged 12 commits into from
Dec 14, 2023
Merged

bug(checkbox): added missing readonly prop value #3859

merged 12 commits into from
Dec 14, 2023

Conversation

Rajdeepc
Copy link
Contributor

@Rajdeepc Rajdeepc commented Dec 8, 2023

Description

Added missing readonly prop value

Related issue(s)

Motivation and context

Showing an error on readonly prop in CheckBox swc-react component.

How has this been tested?

yarn link at packages/checkbox in SWC

and yarn link "@spectrum-web-components/checkbox"

  • Test case 1
    1. Go here
    2. Do this
  • Test case 2
    1. Go here
    2. Do this

Screenshots (if appropriate)

Screenshot 2023-12-08 at 2 22 05 PM

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@Rajdeepc Rajdeepc added bug Something isn't working Component: Checkbox labels Dec 8, 2023
@Rajdeepc Rajdeepc requested a review from Westbrook December 8, 2023 09:16
@Rajdeepc Rajdeepc changed the title chore(checkbox): added missing readonly prop value bug(checkbox)[FIREFLY]: added missing readonly prop value Dec 8, 2023
Copy link

github-actions bot commented Dec 8, 2023

Tachometer results

Chrome

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 509 kB 66.87ms - 69.18ms - unsure 🔍
-4% - +3%
-2.85ms - +1.74ms
branch 495 kB 66.60ms - 70.57ms unsure 🔍
-3% - +4%
-1.74ms - +2.85ms
-

checkbox permalink

Version Bytes Avg Time vs remote vs branch
npm latest 421 kB 99.00ms - 100.36ms - slower ❌
0% - 2%
0.02ms - 1.66ms
branch 407 kB 98.39ms - 99.30ms faster ✔
0% - 2%
0.02ms - 1.66ms
-

switch permalink

Version Bytes Avg Time vs remote vs branch
npm latest 402 kB 27.84ms - 28.15ms - faster ✔
0% - 2%
0.13ms - 0.58ms
branch 388 kB 28.19ms - 28.52ms slower ❌
0% - 2%
0.13ms - 0.58ms
-

table permalink

Version Bytes Avg Time vs remote vs branch
npm latest 537 kB 333.77ms - 340.59ms - unsure 🔍
-1% - +2%
-4.12ms - +5.36ms
branch 500 kB 333.27ms - 339.85ms unsure 🔍
-2% - +1%
-5.36ms - +4.12ms
-
Firefox

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 509 kB 131.06ms - 138.74ms - unsure 🔍
-3% - +5%
-3.50ms - +6.50ms
branch 495 kB 130.20ms - 136.60ms unsure 🔍
-5% - +3%
-6.50ms - +3.50ms
-

checkbox permalink

Version Bytes Avg Time vs remote vs branch
npm latest 421 kB 197.43ms - 208.21ms - unsure 🔍
-5% - +1%
-11.20ms - +3.12ms
branch 407 kB 202.14ms - 211.58ms unsure 🔍
-2% - +6%
-3.12ms - +11.20ms
-

switch permalink

Version Bytes Avg Time vs remote vs branch
npm latest 402 kB 75.08ms - 80.84ms - unsure 🔍
-4% - +7%
-3.10ms - +5.14ms
branch 388 kB 73.98ms - 79.90ms unsure 🔍
-7% - +4%
-5.14ms - +3.10ms
-

table permalink

Version Bytes Avg Time vs remote vs branch
npm latest 537 kB 542.30ms - 557.66ms - unsure 🔍
-1% - +3%
-4.30ms - +18.02ms
branch 500 kB 535.02ms - 551.22ms unsure 🔍
-3% - +1%
-18.02ms - +4.30ms
-

Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Is there a way to trigger the error that raised this? Being typings and not functionality, it seems like maybe not, but it would be good if we could add something that prevented issues like this in future refactors.

@Westbrook
Copy link
Contributor

Related, looks like Spectrum CSS moves the input to disabled in this state, maybe we should do the same?
image

@Rajdeepc Rajdeepc requested a review from Westbrook December 11, 2023 10:09
@Westbrook
Copy link
Contributor

#3859 (review)

Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Is there any way that we can test the specific issue that arose here in your consumption?

@Rajdeepc
Copy link
Contributor Author

Is there any way that we can test the specific issue that arose here in your consumption?

Yes should be added now.

@Rajdeepc Rajdeepc linked an issue Dec 14, 2023 that may be closed by this pull request
1 task
@Rajdeepc Rajdeepc changed the title bug(checkbox)[FIREFLY]: added missing readonly prop value bug(checkbox): added missing readonly prop value Dec 14, 2023
@Rajdeepc Rajdeepc requested a review from Westbrook December 14, 2023 16:20
Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for getting that test in there, will be a big help for future refactoring. 🙇🏼

@Westbrook Westbrook merged commit 35b5649 into main Dec 14, 2023
47 checks passed
@Westbrook Westbrook deleted the bug/checkbox branch December 14, 2023 16:34
najikahalsema pushed a commit that referenced this pull request Dec 19, 2023
* chore(checkbox): added missing prop value

* chore(checkbox): updated disabled on readonly

* chore(checkbox): update golden image cache

* chore(checkbox): allowing support for both setting and removing readonly

* chore(checkbox): added the disabled proprety directly to the input

* chore(checkbox): updated golden image cache

* chore(checkbox): added test for readonly typing

---------

Co-authored-by: Rajdeep Chandra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Component: Checkbox
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Add disabled property to input in checkbox for readonly
2 participants