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

fixed-select-option-using-label-issue #5098

Merged
merged 7 commits into from
Dec 7, 2021

Conversation

Prateek93a
Copy link
Contributor

Resolves #4854

Changes:

I am using the id attribute of the option element to set the for attribute of label element.

I wanted to make minimal changes to the code for fixing the issue. Hence I went with approach that @dipamsen suggested. However if required I will update the code to implement the approach @limzykenneth suggested.

GIF of the change:

ezgif com-gif-maker (2)

PR Checklist

  • npm run lint passes

@limzykenneth
Copy link
Member

@Prateek93a Can the nested label input approach be used here instead? I want to keep the id to be something that the user explicitly define only, otherwise it may cause problem when the user attempts to set an id on the input element.

@Prateek93a
Copy link
Contributor Author

@limzykenneth Yes, I will update the PR.

@Prateek93a
Copy link
Contributor Author

@limzykenneth I have updated the code to use the nested label input approach. For this, I have also made small updates to the unit tests to work according to the changes.

@lmccart
Copy link
Member

lmccart commented Mar 29, 2021

@limzykenneth if this looks good to you, please feel free to merge

@limzykenneth
Copy link
Member

This looks good to me and can be merged though I only just noticed that createCheckbox is using random ID to associate labels with the checkbox as was done here before. @Prateek93a Are you interested in changing the behaviour of createCheckbox to match your implementation here of nested label tag? If so you can push directly to this PR and I'll merge when that's done, otherwise I can merge this as is.

@Prateek93a
Copy link
Contributor Author

@limzykenneth Sure. I will make the changes and update the PR soon.

@Prateek93a
Copy link
Contributor Author

@limzykenneth I have made the changes. I have also added some unit tests for createCheckbox function.

@limzykenneth
Copy link
Member

Sorry this took way too long, it slipped through the cracks. I'll review this today.

@limzykenneth limzykenneth merged commit bd367b4 into processing:main Dec 7, 2021
@limzykenneth
Copy link
Member

Seems to all work fine. We can revisit if specific problems occur. Thanks @Prateek93a!

@Prateek93a
Copy link
Contributor Author

No problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label is not clickable on Radio Buttons with createRadio()
3 participants