Add support for checkboxes and radio buttons #30
Conversation
|
Updated the PR to account for default checked checkboxes that get unchecked, along with a test for it. Hoping this is good to go so I can also move the work along on the actual form implementation side 🤞 |
src/index.ts
Outdated
| function shouldResumeField(field: HTMLInputElement | HTMLTextAreaElement): boolean { | ||
| return !!field.id && field.value !== field.defaultValue && field.form !== submittedForm | ||
| return ( | ||
| !!field.id && | ||
| (field.value !== field.defaultValue || | ||
| (field instanceof HTMLInputElement && field.checked !== field.defaultChecked)) && | ||
| field.form !== submittedForm | ||
| ) | ||
| } |
There was a problem hiding this comment.
This condition is getting pretty dense now. I wonder if we should break it up a little?
There was a problem hiding this comment.
It's not my favorite, no, but IMO it's still okay for the moment as an iterative change. I would definitely rewrite this if we were introducing support for select elements, though. But I don't want to hold up the currently needed change/release over that :)
There was a problem hiding this comment.
@seanpdoyle I've added you to my fork so you can push there and keep it moving! You are also more than welcome to pull my branch into your own fork and take it from there but I figured adding you would be a quick way.
There was a problem hiding this comment.
@helen @theinterned I've pushed up some changes to resolve this branch's conflicts with main.
There was a problem hiding this comment.
Sorry it's taken me so long to review this. I'm looking at it now
This feels a little rough around the edges, but it's a working start.
Includes a button to set sessionStorage for easier testing/debugging
seanpdoyle
force-pushed
the
add/checked-support
branch
from
1772332 to
0ef195d
Compare
seanpdoyle
force-pushed
the
add/checked-support
branch
from
0ef195d to
909d41c
Compare
|
@seanpdoyle the changes make sense, however there are currently several failing tests on this branch. Can you please have a look? Thanks! |
test/test.js
Outdated
| <input id="my-first-field" value="first-field-value" class="js-session-resumable" /> | ||
| <input id="my-second-field" value="second-field-value" class="js-session-resumable" /> |
There was a problem hiding this comment.
I would guess one problem with the tests is that they are looking for input[type=text] and these are missing a type
| <input id="my-first-field" value="first-field-value" class="js-session-resumable" /> | |
| <input id="my-second-field" value="second-field-value" class="js-session-resumable" /> | |
| <input id="my-first-field" type="text" value="first-field-value" class="js-session-resumable" /> | |
| <input id="my-second-field" type="text" value="second-field-value" class="js-session-resumable" /> |
|
UPDATE: @seanpdoyle here's a PR against this branch that fixes the tests helen#1 |
ns/add/checked support
There was a problem hiding this comment.
Thank you @helen and @seanpdoyle for finally getting this over the finish line!
|
🚀 This has shipped in v0.5.0 |
Noticed while adding some requested form support that inputs with a checked state are not accounted for, as they are dropped in the
field.value !== field.defaultValuecheck. This adds support for them! And adds checkboxes to the test. I'm not sure this is the world's most elegant approach, but I think it's easy to understand and doesn't introduce a ton of logic.Also included is an example/test page that I made to check that this wasn't a bug caused by something else in the bigger stack. The page has a button to set the
sessionStorage, as I found it difficult to debug otherwise, but not a submit button. Happy to add a submit button :)