Skip to content

Checkbox, Radio, RadioSet#97

Merged
100stacks merged 3 commits intomasterfrom
Checkbox1
Jul 29, 2020
Merged

Checkbox, Radio, RadioSet#97
100stacks merged 3 commits intomasterfrom
Checkbox1

Conversation

@nicko-winner
Copy link
Contributor

2020-07-18_21-56-01

@netlify
Copy link

netlify bot commented Jul 19, 2020

Deploy preview for helix-react ready!

Built with commit 09b34a6

https://deploy-preview-97--helix-react.netlify.app

const Checkbox = ({ id, label, className, indeterminate, ...rest }) => {
const inputRef = useRef();
useEffect(() => {
inputRef.current.indeterminate = indeterminate;
Copy link
Contributor Author

@nicko-winner nicko-winner Jul 20, 2020

Choose a reason for hiding this comment

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

2020-07-18_20-50-50
the docs say el.intermediate but sine there are many elements that exist to make this checkbox work it might be more clear if the docs said inputEl. indeterminate = true instead to be more explicit.

Copy link
Member

Choose a reason for hiding this comment

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

@nicko-winner, is this feedback for the HelixUI docs or something we should document here (or both)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a nit for the helix UI docs. I might have just have been trying silly things late in the day, but I tried to set the indeterminate prop on a few wrong elements before I got it working on the input. If the docs are explicit about what element to place indeterminate on it might take some ambiguity out of it. Not many people will likely need to use that state, but for those that do maybe someone will try the same foolishness I did.

Copy link
Contributor Author

@nicko-winner nicko-winner Jul 21, 2020

Choose a reason for hiding this comment

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

In Helix React we hide this implementations detail from users, so they won't have to deal with it, just Helix UI users.

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks for the feedback. Yes, I agree we need to make this more explicit in the HelixUI documentation.

@nicko-winner nicko-winner changed the title Checkbox Checkbox, Radio, RadioSet Jul 21, 2020
Copy link
Member

@100stacks 100stacks left a comment

Choose a reason for hiding this comment

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

Dev LGTM...thanks!

const Checkbox = ({ id, label, className, indeterminate, ...rest }) => {
const inputRef = useRef();
useEffect(() => {
inputRef.current.indeterminate = indeterminate;
Copy link
Member

Choose a reason for hiding this comment

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

@nicko-winner, is this feedback for the HelixUI docs or something we should document here (or both)?

@100stacks 100stacks added this to the v1.0.0-rc.0 milestone Jul 21, 2020
@100stacks 100stacks merged commit 98cba20 into master Jul 29, 2020
@100stacks 100stacks deleted the Checkbox1 branch July 29, 2020 19:45
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