hiccup
hiccup copied to clipboard
Implement multi-select with tests
Allow multiple selected options inside
(I added a detailed comment to the function so that someone in the future doesn't try to simplify the code and miss the nuances.)
Is the issue for the commit message that it's too how-oriented or are the lines too short? This is what my git log looks like:
Implement multi-select with tests
Allow multiple selected options inside <select>s with the
`multiple` attribute. Instead of checking equality using
`=`, a `selected?` function is implemented that checks
whether the current value being rendered is found inside
`selected`, which can be a sequence, a function, or a single
value.
Just want to know how to update it to make it better. Also, should I squash all my commits into a single one (including this PR)? If so, I'll need to force-push to my repo, which should be fine since I'm the only one using it. But I'm not sure how that will affect these comments that quote the original.
Advice appreciated. Thanks.
Perhaps something like this:
Allow multiple selected options in select elements
Allow multiple selected options in order to support select elements that
have the 'multiple' attribute. The 'selected' argument can now be a
collection of values or a predicate function, as well as a single value.
I removed the markdown backticks to make it plaintext, changed the line length to 72 characters, and made the text a little clearer.
Force pushing to a PR branch is fine, and GitHub will still allow previous comments on older commits to be seen. Typically I prefer rewriting history within a PR, as otherwise when the PR is merged it adds a lot of unnecessary commits that clutter the commit log.
Done! Thanks for the advice.
Thanks! I'll merge this into master once I release 2.0.0, if that's okay by you. Since I've got a release candidate for 2.0.0, I want to delay any new features until after I've got that out the door.
Sounds good to me! Thanks!
On Sun, Feb 11, 2024 at 10:36 AM James Reeves @.***> wrote:
Thanks! I'll merge this into master once I release 2.0.0, if that's okay by you. Since I've got a release candidate for 2.0.0, I want to delay any new features until after I've got that out the door.
— Reply to this email directly, view it on GitHub https://github.com/weavejester/hiccup/pull/209#issuecomment-1937833296, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU7IAAALM3D3DDGWSHWCALYTEFRZAVCNFSM6AAAAABCZGSBYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZXHAZTGMRZGY . You are receiving this because you authored the thread.Message ID: @.***>