-
Notifications
You must be signed in to change notification settings - Fork 351
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
Set required and aria on input when input is required #606
Set required and aria on input when input is required #606
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious about the missing space in check boxes.
test/bootstrap_form_test.rb
Outdated
<label class="form-check-label" for="user_misc_1"> Foo</label> | ||
<label class="form-check-label" for="user_misc_1">Foo</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has that space disappeared? Maybe I didn't notice this in the PR against main
, and if so, sorry. But I'm curious why it's gone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure that my test failed with the space, but i double checked just know and with or without the space before Foo it succeeds.
Both in my branches as in the main branch.
So I think the space was absolute before already but why the test succeeded before i do not know.
When looking in the browser at a checkbox, the label does not have a space either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't need to change the cases where there's a space before the text, I'd prefer not to change it. The fewer changes, the less likely we inadvertently cause problems for someone who uses bootstrap_form
. Can you make the change back to the way it was, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem fixed it what do you want me to do with the commit already merged to master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just write up an issue describing how Bootstrap 4 tests are different from Bootstrap 5, we can review it later. But if it's more work to do write up an issue than to do another PR for main
, then submit another PR. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is resolved, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is resolved
9564f23
to
d32c9fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this PR! Great work!
This looks like a good change. Any reason not to merge it? @CharlieWinkwaves could you rebase onto master? |
43c0e35
to
4d3e178
Compare
4d3e178
to
306f49b
Compare
I do not know why but is keeps saying that this PR has 4 commits in it |
4d3e178
to
131fc0b
Compare
131fc0b
to
5c01054
Compare
fixes #604