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

Replace usages of old context API #68

Merged
merged 4 commits into from
Feb 26, 2019
Merged

Replace usages of old context API #68

merged 4 commits into from
Feb 26, 2019

Conversation

xadn
Copy link
Collaborator

@xadn xadn commented Feb 3, 2019

Hey guys, I really love this project, thanks for keeping it up to date! The tool I'm building sometimes does a lot of rendering which slows down the rest of the app. Unfortunately, the usage of the old context API means that the inspector component won't work with concurrent rendering and strict mode.

This PR replaces the usages of the old context API ...with hooks. Since it depends on the pre-release version of React this probably shouldn't be merged until 16.8.0.

If you're opposed to using hooks, I'd be happy to modify this to be more in-line with a different direction, but I think this ended up being pretty nice.

Changes

  • Added useStyles(key) hook to replace the createStyles(key, theme) util
  • Added themeAcceptor HOC for root components to take the "theme" prop and set the context
  • Refactored class components to functional components to enable the usage of hooks
    • Replaced defaultProps with default values in those components

Note

To make it easier to review there are two diffs in this PR, one to run prettier on the changed files and a follow up to make the desired changes. I could redo this without prettier if needed.

Example of changes

The TH component does a good job of demonstrating what these changes look like in practice.

Old TH component (50 lines)

class TH extends Component {
  state = { hovered: false };

  toggleHovered(hovered) {
    this.setState({ hovered: hovered });
  }

  render() {
    const {
      borderStyle,
      children,
      onClick,
      sortAscending,
      sorted,
      ...props
    } = this.props;
    const { theme } = this.context;
    const styles = createStyles('TableInspectorTH', theme);

    return (
      <th
        {...props}
        style={{
          ...styles.base,
          ...borderStyle,
          ...(this.state.hovered ? styles.base[':hover'] : {}),
        }}
        onMouseEnter={this.toggleHovered.bind(this, true)}
        onMouseLeave={this.toggleHovered.bind(this, false)}
        onClick={onClick}>
        <div style={styles.div}>{children}</div>
        {sorted && (
          <SortIconContainer>
            <SortIcon sortAscending={sortAscending} />
          </SortIconContainer>
        )}
      </th>
    );
  }
}

TH.contextTypes = {
  theme: PropTypes.oneOfType([PropTypes.string, PropTypes.object]).isRequired,
};

TH.defaultProps = {
  sortAscending: false,
  sorted: false,
  onClick: undefined,
};

New TH component (34 lines)

const TH = ({
  sortAscending = false, // <= REPLACES defaultProps
  sorted = false,
  onClick = undefined,
  borderStyle = {},
  children,
  ...thProps
}) => {
  const styles = useStyles('TableInspectorTH'); // <= HOOK READS THE THEME CONTEXT
  const [hovered, setHovered] = useState(false); // <= HOOK FOR STATE

  // v= NO NEED TO BIND THE CALLBACK
  const handleMouseEnter = useCallback(() => setHovered(true), []);   
  const handleMouseLeave = useCallback(() => setHovered(false), []);

  return (
    <th
      {...thProps}
      style={{
        ...styles.base,
        ...borderStyle,
        ...(hovered ? styles.base[':hover'] : {}),
      }}
      onMouseEnter={handleMouseEnter}
      onMouseLeave={handleMouseLeave}
      onClick={onClick}>
      <div style={styles.div}>{children}</div>
      {sorted && (
        <SortIconContainer>
          <SortIcon sortAscending={sortAscending} />
        </SortIconContainer>
      )}
    </th>
  );
};

@xyc
Copy link
Collaborator

xyc commented Feb 7, 2019

Hi @xadn , thanks for this awesome PR! I'm happy to see it uses hooks, especially hooks are now in a stable react release in 16.8.0. This would have to be a breaking change though, and we need to take the cost of maintaining two versions of the lib into consideration.

@ndelangen This is a breaking change. If you could chime in here it'd be great.

I've also thought of introducing some new changes in a breaking release (#50), it'll be a good time to review it.

package.json Outdated
"react": "^16.3.1",
"react-dom": "^16.3.1",
"prettier": "^1.16.3",
"react": "^16.8.0-alpha.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

peer dependencies need to be updated too.

/**
* Hook to get the component styles for the current theme.
*/
export const useStyles = key => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to baseStylesKey and add a little comment/jsdoc? Might help clarify the usage.

@xyc
Copy link
Collaborator

xyc commented Feb 7, 2019

Probably we don't want include yarn-error.log. Could we add it to .gitignore?

export const themeAcceptor = Component => {
const ThemeAcceptor = ({ theme = DEFAULT_THEME_NAME, ...restProps }) => {
const themeStyles = useMemo(() => {
switch (Object.prototype.toString.call(theme)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

* the current theme. This is intended to be used by the top-level inspector
* components.
*/
export const themeAcceptor = Component => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, maybe rename to WrappedComponent, not to confuse with React.Component

@xadn
Copy link
Collaborator Author

xadn commented Feb 10, 2019

Thanks for the review, I'll fix these up today.

…tyles.js), upgrade to stable version of react
Copy link
Collaborator

@xyc xyc left a comment

Choose a reason for hiding this comment

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

LGTM, will prepare for a release this weekend

@ndelangen
Copy link
Member

🎉

@xyc xyc merged commit 9209262 into storybookjs:master Feb 26, 2019
@xyc
Copy link
Collaborator

xyc commented Feb 26, 2019

@xadn thanks for the work, it was awesome! It's not released yet; I'll fix up some minor issues and release as 3.0.0

This was referenced Feb 28, 2019
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