Implement multi-select with tests

[xuanwu]

Allow multiple selected options inside 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.

(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.)

xuanwu commented 8 months ago

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.

weavejester commented 8 months ago

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.

xuanwu commented 8 months ago

Done! Thanks for the advice.

weavejester commented 7 months ago

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.

xuanwu commented 7 months ago

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: @.***>

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[xuanwu]

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.

[weavejester]

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.

[xuanwu]

Done! Thanks for the advice.

[weavejester]

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.

[xuanwu]

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: @.***>