hiccup icon indicating copy to clipboard operation
hiccup copied to clipboard

Implement multi-select with tests

Open xuanwu opened this issue 1 year ago • 5 comments

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

xuanwu avatar Feb 04 '24 22:02 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.

xuanwu avatar Feb 07 '24 06:02 xuanwu

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.

weavejester avatar Feb 08 '24 16:02 weavejester

Done! Thanks for the advice.

xuanwu avatar Feb 09 '24 04:02 xuanwu

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.

weavejester avatar Feb 11 '24 18:02 weavejester

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

xuanwu avatar Feb 12 '24 09:02 xuanwu