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
Adding ui-checkbox element. #31
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -195,3 +195,24 @@ | |
(dissoc :active :title :type) | ||
clj->js)] | ||
(apply dom/div attributes (when title (build-title title)) children))) | ||
|
||
(defn ui-checkbox | ||
"Render a checkbox (not the label). Props is a normal clj(s) map with React/HTML attributes plus: | ||
|
||
className (optional): additional class stylings to apply to the top level of the checkbox | ||
id: Name of the checkbox | ||
style (optional): is-indeterminate :c-checkbox--informative" | ||
[{:keys [id style] :or {id ""} :as attrs}] | ||
(let [legal-styles #{:is-indeterminate :c-checkbox--informative} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Informative has been removed as it's an internal-only class we don't need in this project. |
||
user-classes (get attrs :className "") | ||
classes (cond-> (str user-classes " c-checkbox ") | ||
(contains? legal-styles style) (str (name style)) | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style guide: no closing brace of any kind should start the non-whitespace of a line. Join this up to the line above. |
||
attrs (cond-> attrs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop the Also assoc and dissoc can do more than one at a time:
|
||
:always (assoc :type "checkbox") | ||
:always (dissoc :styles) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type: I think you mean style...but I would not use that, since it has an HTML meaning. See above comments on API of this method. |
||
:always (assoc :className classes) | ||
:always (assoc :id (name id))) ] | ||
(dom/span #js {} (dom/input (clj->js attrs)) | ||
(dom/label #js {:htmlFor id} \u00A0)) | ||
)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dangling parens |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,3 +80,10 @@ | |
(e/ui-message {:color :warning} "This is a warning message.") | ||
(e/ui-message {:color :warning} "This is a warning message with another child." (icon :arrow_forward)) | ||
(e/ui-message {:className "h2"} "This is message using a standard H2 class name."))) | ||
|
||
(defcard checkbox | ||
(dom/div nil | ||
(e/ui-checkbox {:id "checkbox"} ) | ||
(e/ui-checkbox {:id "checkbox" :style #{:is-indeterminate}} ) | ||
(e/ui-checkbox {:id "checkbox" :style #{:c-checkbox--informative}} ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API should never talk about CSS classes (you're mixing a CSS stye name into a user-visible API). It should be abstract. |
||
(e/ui-checkbox {:id "checkbox" :style #{:is-indeterminate :c-checkbox--informative}} ))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you want is:
:checked
- Can be one oftrue
,false
, or:indeterminate
that way it is compatible with normal checkbox use (true/false), but can support the additional case.
:style
should never be used in our elements, since it is an HTML predefined attribute.You can use
(identical? true v)
to check that something is the actual boolean value true (or false for that matter).