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 issue where fullscreen change didn't cause re-render #1583

Merged

Conversation

OskarDamkjaer
Copy link
Contributor

A performance optimization caused the VisualizationView.tsx not to rerender as the fullscreen prop changed. Causing this issue:

CleanShot.2021-11-02.at.15.08.04.mp4

Preview of the fix @ http://zoombuttons_fullscreen_issue.surge.sh

Copy link
Contributor

@jchen042 jchen042 left a comment

Choose a reason for hiding this comment

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

I haven't pulled the code locally but the fix looks reasonable to me.

A general question: do we plan to refactor our class components to functional components as possible to separate container and presentational components in the future? @OskarDamkjaer @jharris4

@jharris4
Copy link
Collaborator

jharris4 commented Nov 2, 2021

@FallingCeilingS choosing class vs functional components is mostly a matter of style, but also in some cases using one leads to cleaner & simpler code than the other.

For working on style improvements in the future, we would like to refactor some of our components to be "dumb" instead of "smart".

"dumb" in this context that all data or callbacks are passed in as props, and any redux type logic is separated into wrapping "smart" components.

Copy link
Collaborator

@jharris4 jharris4 left a comment

Choose a reason for hiding this comment

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

LGTM

@OskarDamkjaer OskarDamkjaer merged commit 851170c into neo4j:master Nov 8, 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