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

refactor(react): consolidate useControllableState to one hook #10255

Conversation

joshblack
Copy link
Contributor

This PR consolidates our two versions of useControllableState into one hook:

/**
 * This custom hook simplifies the behavior of a component if it has state that
 * can be both controlled and uncontrolled. It functions identical to a
 * useState() hook and provides [state, setState] for you to use. You can use
 * the `onChange` argument to allow updates to the `state` to be communicated to
 * owners of controlled components.
 *
 * Note: this hook will warn if a component is switching from controlled to
 * uncontrolled, or vice-verse.
 *
 * @param {object} config
 * @param {string} config.name - the name of the custom component
 * @param {any} config.defaultValue - the default value used for the state. This will be
 * the fallback value used if `value` is not defined.
 * @param {Function} config.onChange - an optional function that is called when
 * the value of the state changes. This is useful for communicating to parents of
 * controlled components that the value is requesting to be changed.
 * @param {any} config.value - a controlled value. Omitting this means that the state is
 * uncontrolled
 * @returns {[any, Function]}
 */
export function useControllableState({
  defaultValue,
  name = 'custom',
  onChange,
  value,
}) {

It also updates components that used the old form of this hook and updated import paths for components that used the new form.

Changelog

New

Changed

  • Merge different useControllabeState hooks into one hook
  • Update import paths to useControllableState
  • Update usage of old hook to new API

Removed

Testing / Reviewing

  • Review the new implementation and see how it compares to the previous version of this hook
  • Verify tests pass as expected
  • Verify functionality is not lost in the following components that have been updated:
    • next/ContentSwitcher
    • next/Toggle
    • next/Tabs

@joshblack joshblack requested a review from a team as a code owner December 8, 2021 18:52
@netlify
Copy link

netlify bot commented Dec 8, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: e3df732

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61b283fd0f61470008a5935e

😎 Browse the preview: https://deploy-preview-10255--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Dec 8, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: e3df732

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61b283fd0f2a2500074078cc

😎 Browse the preview: https://deploy-preview-10255--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Dec 8, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: e3df732

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61b283fd16f8e400086df89f

😎 Browse the preview: https://deploy-preview-10255--carbon-components-react.netlify.app

Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

LGTM. Tests are passing and the components are working as expected. 👍🏽

warning(
false,
'A component is changing a controlled component to be uncontrolled. ' +
'A component is changing a controlled %s component to be uncontrolled. ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

what does %s mean again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

%s is a convention used in warning, it comes from sprintf style formatters in languages like C. You can also do it in things like console.log(), for example:

console.log('Hello, %s', 'world'); // 'Hello, world'

Basically it's a way of interpolating values into a string

*/
export function useControllableState({
defaultValue,
name = 'custom',
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what would an example of name being used be? Would it be the name of the state being used or the component that is being controlled?

Copy link
Contributor Author

@joshblack joshblack Dec 9, 2021

Choose a reason for hiding this comment

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

@abbeyhrt great question, it'd be the latter. The idea with name was that a component could provide the name argument to offer better development error messages. So, for example, Tabs could useControllableState({ name: 'Tabs' }) and it would say that "a Tabs component is switching from controlled to uncontrolled"

@kodiakhq kodiakhq bot merged commit 8a4958e into carbon-design-system:main Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants