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

Support select elements #55

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Feb 1, 2024

First, introduce the PersistableElement type to incorporate <input>,
<textarea>, and <select> elements.

Next, incorporate some special handling for HTMLSelectElement, since
they don't have a .defaultValue property.

Support for HTMLSelectElement instances is generalized for both
single and [multiple] elements by looping over
HTMLSelectElement.selectedOptions while persisting, then setting
HTMLSelectElement.selected when restoring.

@seanpdoyle seanpdoyle requested a review from a team as a code owner February 1, 2024 19:52
@seanpdoyle seanpdoyle requested a review from mattcosta7 February 1, 2024 19:52
@seanpdoyle
Copy link
Contributor Author

@theinterned this work aims to complement #30.

Copy link
Contributor

@theinterned theinterned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good!

Before I approve, Can you please add some docs to the readme about select support and call out the gap with multi-selects?

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@seanpdoyle seanpdoyle force-pushed the html-select-element-support branch from 71a2686 to 4d5340d Compare February 7, 2024 14:31
@seanpdoyle seanpdoyle changed the title Support select:not([multiple]) elements Support select elements Feb 7, 2024
@seanpdoyle
Copy link
Contributor Author

@theinterned I've incorporated your suggestion on looping over selectedOptions to support both single and [multiple] elements.

@theinterned
Copy link
Contributor

@seanpdoyle there are conflicts since merging #30 — I'll give this a review once those are resolved. Thank you 🙏

First, introduce the `PersistableElement` type to incorporate `<input>`,
`<textarea>`, and `<select>` elements.

Next, incorporate some special handling for `HTMLSelectElement`, since
they don't have a `.defaultValue` property.

Support for `HTMLSelectElement` instances is generalized for both
single and `[multiple]` elements by looping over
[HTMLSelectElement.selectedOptions][] while persisting, then setting
[HTMLSelectElement.selected][] when restoring.

[HTMLSelectElement.selectedOptions]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement/selectedOptions
[HTMLSelectElement.selected]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLOptionElement#instance_properties
@seanpdoyle seanpdoyle force-pushed the html-select-element-support branch from 4d5340d to 8154f26 Compare February 8, 2024 14:36
@seanpdoyle
Copy link
Contributor Author

@theinterned I've pushed up a rebased commit. Thank you!

Copy link
Contributor

@theinterned theinterned 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 this 🎉

I'll cut a release

.filter(field => shouldResumeField(field, storageFilter))
.map(field => {
if (field instanceof HTMLSelectElement) {
return [field.id, Array.from(field.selectedOptions).map(option => option.value)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah cool using selectedOptions is really nice! I didn't realize it worked equally for multiple and non-multiple selects.

@theinterned theinterned merged commit 7b4243a into github:main Feb 8, 2024
1 check passed
@theinterned
Copy link
Contributor

@seanpdoyle seanpdoyle deleted the html-select-element-support branch February 8, 2024 20:51
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.

2 participants