Skip to content

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented May 15, 2019

WHY are these changes introduced?

Part of #1484

WHAT is this pull request doing?

I'm removing withContext in favor of a wrapper component

How to 🎩

Tests / percy / development server

@BPScott BPScott temporarily deployed to polaris-react-pr-1503 May 15, 2019 21:44 Inactive
@AndrewMusgrave AndrewMusgrave added the 🤖Skip Changelog Causes CI to ignore changelog update check. label May 15, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-1503 May 15, 2019 21:52 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1503 May 15, 2019 21:56 Inactive
@AndrewMusgrave
Copy link
Member Author

A lot of the lacking coverage has been removed in master with simpler interaction states. So when master is merged into version-4.0.0 it should be 👌

@AndrewMusgrave AndrewMusgrave requested a review from alex-page May 15, 2019 22:47
</div>
);

function handleAnchorFocus() {
Copy link
Member

@BPScott BPScott May 16, 2019

Choose a reason for hiding this comment

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

Having a bunch of functions defined beneath the return feels odd to me. Can you pull these up to be towards the start of the function.

This is a functional component and callback functions that we define need to be wrapped in useCallback. Otherwise a new callback function ends up being created every render, and when you pass that down to a child component you end up rerendering it - as its props have changed becase it's onWhatever handler is a different function to the one that was used before.

Wrapping these declarations in useCallback means the same function will be passed to the child component if given the same arguments, thus avoiding unneeded rerenders.

const handleFocusedBlur  = useCallback(() => {
   setFocusState({focused: true, focusedInner: true});
  }, []);

See BPScott/ksw-quiz@30b4c3f for an example of how I dealt with this in my little toy project for playing with react

Copy link
Member Author

Choose a reason for hiding this comment

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

I find putting the functions below cleans up the code, Tim does the same as well I think. However, if we use functional expressions we'll want to pull it up. Thanks for pointing this out, I was worried about it but wasn't aware there was a hook to solve it 🙏

@BPScott BPScott temporarily deployed to polaris-react-pr-1503 May 16, 2019 22:22 Inactive
Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

🎩looks good to me.

@alex-page alex-page requested a review from BPScott May 28, 2019 14:37
@BPScott BPScott temporarily deployed to polaris-react-pr-1503 May 28, 2019 17:46 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants