Skip to content

Conversation

Andarist
Copy link
Contributor

As useToggleState({}) works just fine it seems reasonable to make props optional.

@snowystinger
Copy link
Member

When would someone use it like this?

My thoughts are if it's empty, then there's no onChange, no default, and no controlling, without a lot of extra work. Even the readonly logic would end up redundant.

@Andarist
Copy link
Contributor Author

When would someone use it like this?

When just wanting to use the simplest, uncontrolled, case: https://codesandbox.io/s/nostalgic-rhodes-shve1?file=/src/App.js . This works just OK when an empty object gets passed in, so it means that it is not actually needed to be given explicitly. The default value is handled internally with a fallback here: https://github.com/adobe-private/react-spectrum-v3/blob/a32224fdf009d69cf594f32ad18ac86d7ebb8a52/packages/%40react-stately/toggle/src/useToggleState.ts#L32

@snowystinger
Copy link
Member

Given that the intent is really for props to be passed to the hook, I'm not sure we want to encourage not doing that.
Also, none of our hooks make this optional and we'd probably want to do that to be consistent. I'm not sure we should do that though.

@devongovett devongovett changed the base branch from master to main July 12, 2020 00:53
@devongovett
Copy link
Member

Seems fine to me. We should be consistent across all hooks that don't have any required props. Happy to merge this one and do an audit of other hooks separately if you want!

@Andarist
Copy link
Contributor Author

@devongovett I would still consider this PR as sound. I'm not sure if it's worth doing an audit - such restrictions can be lifted when requested (would save you some time to not do the audit all at once), but that's, of course, up to you.

@devongovett
Copy link
Member

Ok thanks @Andarist! We'll take a look and see what other hooks should support this.

@devongovett devongovett merged commit 023ea5e into adobe:main Jul 17, 2020
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.

3 participants