-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 Fix: Devise Checkbox TRUE VALUES not compatible with HTML specs (missing "on" value) #5514
Conversation
Ping! Any progress on this? |
@carlosantoniodasilva I think this project needs new contributors or it's of risk of dying. Do you need help? |
@itay-grudev thank you! Could you please add 'ON' as well to this list? Looks like Rails decided to use FALSE_VALUES (instead of TRUE_VALUES) at some point (https://github.com/rails/rails/blob/c7de35a41bd7255249c9a5750e6a6edf75e61c82/activemodel/lib/active_model/type/boolean.rb#L15) but I think we can keep it as it is in Devise for now. |
@nashby Done. |
@nashby I rebased against main. This probably will fix the test failures, as they cannot possibly be caused by my change. Could you approve the workflow? |
@itay-grudev thanks, failing specs are not related to this change so this is fine. Could you please squash your commits? |
GitHub does that when you click merge and create one merge commit on the entire PR diff. |
If not - consider enabling it. It keeps your repo clean without using extra human time. |
@itay-grudev the thing is that I want to have a merge commit but I don't want to have 3 commits on top (it's fine to have multiple commits in one PR but not in this case). So the only way to do it if you squash them, sorry for the trouble |
…missing "on" value) See: * https://html.spec.whatwg.org/multipage/input.html#checkbox-state-(type%3Dcheckbox) * https://html.spec.whatwg.org/multipage/input.html#dom-input-value-default-on This issue causes the remember me functionality not to work correctly, especially when overriding the default styles as check boxes by default in all major browsers send the value `on` as required by the HTML specifications. See also: https://stackoverflow.com/questions/5987075/devise-remember-me-not-working-localhost-issue
I wish to inform you that because of the non-linear commits applied out of order over the time-span of over 2 years - it took me 10min to understand / resolve the situation and squash them. And I am git skilled. I knew what I was doing. This is as opposed to the UI-based squash merge that would have taken less than 30s if enabled. As a true software engineer I despise this bureaucratic inefficiency, hence this complaint. |
@nashby Done. |
See:
This issue causes the remember me functionality not to work correctly, especially when overriding the default styles as check boxes by default in all major browsers send the value
on
as required by the HTML specifications.See also:
https://stackoverflow.com/questions/5987075/devise-remember-me-not-working-localhost-issue