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

fix: Link - LinkWrapper optimisation #44

Merged
merged 2 commits into from
Jun 29, 2019

Conversation

jsomsanith
Copy link
Collaborator

What is the problem this PR is trying to solve?
Link component accepts a LinkWrapper props. It is styled via styled-component, but it’s not exactly LinkWrapper that is passed, but a new component that omits a set of props.
But everytime Link renders, the styled LinkWrapper is created again, defining a new component.

React considers it as a new component, the old button will be unmounted, and the new one will be mounted, at every render.

What is the chosen solution to this problem?
Memoize the styled LinkWrapper component.

Bonus: the weird Link story with a button as child is removed, inn favor of Button component with ButtonWrapper

@kylesuss kylesuss force-pushed the jsomsanith/fix/link_wrapper_optimisation branch from 0cac904 to b16f5df Compare June 29, 2019 03:48
Copy link
Collaborator

@kylesuss kylesuss left a comment

Choose a reason for hiding this comment

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

💥 💥 💥 💥 💥 Nice optimization! Need to be better about this myself.

${linkStyles};
`
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯 good call

@kylesuss kylesuss merged commit f65acb2 into master Jun 29, 2019
@kylesuss kylesuss deleted the jsomsanith/fix/link_wrapper_optimisation branch June 29, 2019 04:29
@kylesuss
Copy link
Collaborator

🚀 PR was released in v0.0.35 🚀

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

Successfully merging this pull request may close these issues.

2 participants