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.

Sign up for GitHub

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

Jump to bottom

fix: resolve a11y warnings for label-has-associated-control rule #4152

Open
wants to merge 10 commits into
base: beta
Choose a base branch
from
Open

fix: resolve a11y warnings for label-has-associated-control rule #4152

wants to merge 10 commits into from

Conversation

shubhamchasing
Copy link
Contributor

@shubhamchasing shubhamchasing commented Oct 3, 2024 •
edited
Loading

Description

Fixes warnings for rule - jsx-a11y/label-has-associated-control

Related Tickets & Documents

Fixes #4126

Mobile & Desktop Screenshots/Recordings

Before
Screenshot from 2024-10-03 23-26-52

After
Screenshot from 2024-10-03 23-25-43

Steps to QA

1 run npm run lint in the root folder and see warnings are not occurring.

Tier (staff will fill in)

  • Tier 1
  • Tier 2
  • Tier 3
  • Tier 4

[optional] What gif best describes this PR or how it makes you feel?

de17e41fa1f435a09ab209e9b4939176

@netlify Netlify
Copy link

netlify bot commented Oct 3, 2024 •
edited
Loading

✅ Deploy Preview for oss-insights ready!

Name Link
🔨 Latest commit c511e7f
🔍 Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/6723e299acdf0100085ae8cb
😎 Deploy Preview https://deploy-preview-4152--oss-insights.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 configuration.

@netlify Netlify
Copy link

netlify bot commented Oct 3, 2024 •
edited
Loading

✅ Deploy Preview for design-insights ready!

Name Link
🔨 Latest commit c511e7f
🔍 Latest deploy log https://app.netlify.com/sites/design-insights/deploys/6723e29927008d00088b5c83
😎 Deploy Preview https://deploy-preview-4152--design-insights.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 configuration.

@shubhamchasing shubhamchasing requested a review from jpmcb as a code owner October 13, 2024 01:40
@nickytonline
Copy link
Member

Hi @shubhamchasing, just seeing if you're blocked on anything. No rush on this, just checking in.

@shubhamchasing
Copy link
Contributor Author

My apologies @nickytonline I will submit the changes before this weekend, thanks for offering help

@nickytonline
Copy link
Member

My apologies @nickytonline I will submit the changes before this weekend, thanks for offering help

No need to apologize. 😎 I was just checking in.

@shubhamchasing
Copy link
Contributor Author

shubhamchasing commented Oct 24, 2024 •
edited
Loading

@nickytonline It seems I made some mistake I merged the latest changes to my fix branch and resolved the conflicts but components/organisms/UserSettingsPage/developer-pack-form.tsx empty file is still there, can I delete the empty file in this PR or what can be other solution?

@nickytonline
Copy link
Member

components/organisms/UserSettingsPage/developer-pack-form.tsx no longer exists so if you merge latest into your branch, it should remove it. If you run into issues, delete the file in your PR.

@shubhamchasing
Copy link
Contributor Author

shubhamchasing commented Oct 26, 2024 •
edited
Loading

components/organisms/InsightPage/InsightPage.tsx I think this file is also not used anywhere, can you please confirm?

Edit: I think the below files are unused too or I am making a mistake to find them, please confirm

  • components/molecules/FilterCardSelect/filter-card-select.tsx used in components/molecules/FilterHeader/filter-header.tsx it is unused

  • components/molecules/SelectReportsFilter/select-reports-filter.tsx used in components/organisms/Reports/reports.tsx but it is unused and its related files

  • components/molecules/TeamMemberRow/team-member-row.tsx used in components/molecules/TeamMembersConfig/team-members-config.tsx it is unused

please let me know what files to remove so I do not remove any file which is used somewhere

@nickytonline
Copy link
Member

components/organisms/InsightPage/InsightPage.tsx I think this file is also not used anywhere, can you please confirm?

@shubhamchasing, yes, the components/organisms/InsightPage/InsightPage.tsx can be removed as well.

@shubhamchasing
Copy link
Contributor Author

please check the edited comment #4152 (comment)

@shubhamchasing
Copy link
Contributor Author

shubhamchasing commented Oct 28, 2024 •
edited
Loading

components/organisms/InsightPage/InsightPage.tsx I think this file is also not used anywhere, can you please confirm?

Edit: I think the below files are unused too or I am making a mistake to find them, please confirm

  • components/molecules/FilterCardSelect/filter-card-select.tsx used in components/molecules/FilterHeader/filter-header.tsx it is unused
  • components/molecules/SelectReportsFilter/select-reports-filter.tsx used in components/organisms/Reports/reports.tsx but it is unused and its related files
  • components/molecules/TeamMemberRow/team-member-row.tsx used in components/molecules/TeamMembersConfig/team-members-config.tsx it is unused

please let me know what files to remove so I do not remove any file which is used somewhere

@nickytonline I understand you must be busy but I wanted to close this PR as I took more time on this PR than I should have

@brandonroberts
Copy link
Contributor

brandonroberts commented Oct 30, 2024 •
edited
Loading

Apologies @shubhamchasing. Did you mean close as in complete, or no longer work on?

@shubhamchasing
Copy link
Contributor Author

I will complete, just waiting for Nick's input

@nickytonline nickytonline force-pushed the fix-a11y-label-has-associated-control-warning branch from e48891e to 2c86e3e Compare October 31, 2024 19:43
nickytonline
nickytonline approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member

@nickytonline nickytonline 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 working on this @shubhamchasing. I made a couple tweaks and updated the branch. This is good to go. Thanks for your patience on this one! :shipit:

@shubhamchasing
Copy link
Contributor Author

Thanks @nickytonline, I learned something new 🚀

@shubhamchasing
Copy link
Contributor Author

Hey guys, I was just checking if any help is needed on this to successfully merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CorinaMurg CorinaMurg CorinaMurg left review comments

@nickytonline nickytonline nickytonline approved these changes

@zeucapua zeucapua zeucapua approved these changes

@brandonroberts brandonroberts brandonroberts approved these changes

@jpmcb jpmcb Awaiting requested review from jpmcb jpmcb is a code owner

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

enhancement: a11y rule "label-has-associated-control" fails with a warning.
5 participants
@shubhamchasing @nickytonline @brandonroberts @zeucapua @CorinaMurg