Skip to content
This repository has been archived by the owner on Jul 28, 2020. It is now read-only.

Convert all components to Hooks #766

Closed
35 tasks done
conor-cafferkey-sociomantic opened this issue Jan 23, 2019 · 6 comments
Closed
35 tasks done

Convert all components to Hooks #766

conor-cafferkey-sociomantic opened this issue Jan 23, 2019 · 6 comments

Comments

@conor-cafferkey-sociomantic
Copy link
Contributor

conor-cafferkey-sociomantic commented Jan 23, 2019

It’s time to hooks-ify all the things:

For most components, this just means converting (back) to function components and using:

  • useContext to consume the theme (all of the above components)
  • useImperativeHandle/forwardRef to expose a focus() function (for components that are focusable; remove any props like inputRef, buttonRef, etc.)

For stateful components:

  • event handler functions should be implemented using useCallback
  • derived states calculated in getDerivedStateFromProps can be replaced with useMemo.
@conor-cafferkey-sociomantic
Copy link
Contributor Author

@conor-cafferkey-sociomantic
Copy link
Contributor Author

Hey @daniel-martic-sociomantic, @mahmoud-zakria-sociomantic: I think we should use a custom hook for consuming the theme.

I’m thinking something like this, it would be used in place of useContext :

import ThemeProvider from '.....'

function useTheme( props )
{
    const { displayName } = useTheme.caller; // `caller` is “nonstandard” JS... should we use it? 😈
    const { [ displayName ] : theme } = useContext( ThemeProvider ); // check out ma fancy destructuring!

    return props.cssMap || // cssMap prop overrides the theme, if defined
        ( typeof theme === 'function' ?  theme( props ) : theme ); // if theme is a function, call it with the current props
}

What do you think?

@conor-cafferkey-sociomantic
Copy link
Contributor Author

Right now we have two big-ish problems:

  • Enzyme’s shallow render doesn’t support hooks yet, so all our shallow tests will fail... not a whole lot we can do about this except wait for Enzyme to support Hooks 😕
  • Our test driver suite for Enzyme full-DOM rendering is broken. Currently the component drivers assume that all components are classes not functions. This is fixable and I will open opened an issue.

@mahmoud-zakria-sociomantic
Copy link
Contributor

Hey @daniel-martic-sociomantic, @mahmoud-zakria-sociomantic: I think we should use a custom hook for consuming the theme.

I’m thinking something like this, it would be used in place of useContext :

import ThemeProvider from '.....'

function useTheme( props )
{
    const { displayName } = useTheme.caller; // `caller` is “nonstandard” JS... should we use it? 😈
    const { [ displayName ] : theme } = useContext( ThemeProvider ); // check out ma fancy destructuring!

    return props.cssMap || // cssMap prop overrides the theme, if defined
        ( typeof theme === 'function' ?  theme( props ) : theme ); // if theme is a function, call it with the current props
}

What do you think?

@conor-cafferkey-sociomantic Why would we do it? what the problem with useContext?

@conor-cafferkey-sociomantic
Copy link
Contributor Author

what the problem with useContext?

Absolutely no problem with useContext (this custom hook uses it!)

The problem is having to write something like this in every component is error-prone:

import { ThemeContext } from '....';
import { createCssMap } from '....';
....
const context = useContext( ThemeContext );
const {
    ....,
    cssMap = createCssMap( context.Button, props),
    ....,
} = this.props;
....

In particular, the part where we have to specify the component name (here, Button).

@conor-cafferkey-sociomantic
Copy link
Contributor Author

And while we’re at it, we can do something similar when we generate a component id:

import { generateId } from '....';

function useId( props )
{
    const { displayName } = useId.caller; // 😈
    const id = useMemo(
        () => props.id || generateId( displayName ),
        [ props.id ], // only update the return value if `props.id` changes
    ) ;
    return id;
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants