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

Implement multi-select with tests #209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xuanwu
Copy link

@xuanwu xuanwu commented Feb 4, 2024

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

Copy link
Owner

@weavejester weavejester 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 the PR. I've reviewed it and have a few suggestions. Could you also update the commit message to follow the seven rules of a great git commit message.

Comment on lines 77 to 83
;; According to the HTML spec, the value inside the boolean `selected` attribute
;; can only be "" or "selected" (case insensitive). Hence, the coercion to
;; boolean is required (otherwise, the attribute value will be `x` itself).
;; We also need the `(not (sequential? selected))` requirement in the
;; second `and` because if `selected` is a vector where `x` is not an element,
;; the first `and` will fail, and the second will try to run something
;; like `([a b] c)`, and can easily lead to java.lang.IndexOutOfBoundsException.
Copy link
Owner

Choose a reason for hiding this comment

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

This comment can be part of the pull request. I don't think it needs to be embedded in the code as it's not going to be relevant information for most people reading it.

Copy link
Author

Choose a reason for hiding this comment

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

Got it. Will remove.

Comment on lines 85 to 88
(or
(and (sequential? selected) (boolean ((set selected) x)))
(and (not (sequential? selected)) (ifn? selected) (selected x))
(= x selected)))
Copy link
Owner

@weavejester weavejester Feb 6, 2024

Choose a reason for hiding this comment

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

Given the purpose of this expression, perhaps a better option is to use cond:

(defn- selected? [x selected]
  (cond
    (sequential? selected) (boolean ((set selected) x))
    (ifn? selected) (selected x)
    :else (= x selected)))

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Tests pass as well.

;; like `([a b] c)`, and can easily lead to java.lang.IndexOutOfBoundsException.
(defn- selected? [x selected]
(or
(and (sequential? selected) (boolean ((set selected) x)))
Copy link
Owner

Choose a reason for hiding this comment

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

Why not place boolean outside the or? That way it would also guard against selected values that are functions.

Copy link
Author

Choose a reason for hiding this comment

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

I think your cond suggestion is better at conveying intent, so I will use that. I'll also put boolean outside.

@xuanwu
Copy link
Author

xuanwu commented Feb 7, 2024

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
Copy link
Owner

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
Copy link
Author

xuanwu commented Feb 9, 2024

Done! Thanks for the advice.

Comment on lines 78 to 82
(boolean
(cond
(sequential? selected) ((set selected) x)
(ifn? selected) (selected x)
:else (= x selected))))
Copy link
Owner

Choose a reason for hiding this comment

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

Almost perfect! Could you change this to:

  (boolean (cond
             (sequential? selected) ((set selected) x)
             (ifn? selected) (selected x)
             :else (= x selected))))

Some Clojure editors default to using two spaces to indent functions, but Hiccup uses the style in the Clojure Style Guide, so:

(foo
 (bar x)
 (baz y))

Or:

(foo (bar x)
     (baz y))

Two-space indentation is reserved for control-flow macros like cond. I should probably add a formatting step to the GitHub workflow to make this automatic.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I see. Done.

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.
@weavejester
Copy link
Owner

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
Copy link
Author

xuanwu commented Feb 12, 2024 via email

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