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

autoResize option enabled #24

Closed
wants to merge 2 commits into from
Closed

autoResize option enabled #24

wants to merge 2 commits into from

Conversation

ZachHaber
Copy link
Collaborator

@ZachHaber ZachHaber commented Feb 16, 2022

Fixes #7

It's not the prettiest way of doing this (a lot of flashes), but I have another branch I set up with a bunch of changes to enable an external resize-observer approach.

I set autoResize: true by default as having it not resize would be a breaking change, unfortunately.

For reference, here's the changes I set up to do the resize-observer approach, and if you decide we should go that route, I'll redo that branch with only the changes needed, and better commit messages. Note, there was a need to surface both the networkRef and the div's ref for this.
https://github.com/Wokstym/react-vis-graph-wrapper/compare/resize-observer

@ZachHaber ZachHaber requested review from Wokstym and WikKam February 16, 2022 04:17
@Wokstym
Copy link
Owner

Wokstym commented Feb 21, 2022

I'm much bigger fan of approach shown on resize-observer branch - purely based on looks.

Could you explain what is the purpose of innerRef seen in ResponsiveVisGraph? Also remember to change example in readme if we go that way

@ZachHaber
Copy link
Collaborator Author

I'm much bigger fan of approach shown on resize-observer branch - purely based on looks.

That's entirely fair, it definitely looks nicer, but is more complex to set up. The main part is that we can just indicate that there is a responsive option in the base library, even if it looks bad. It'll be more performant on worse computers since they are debouncing the change events significantly.

Could you explain what is the purpose of innerRef seen in ResponsiveVisGraph? Also remember to change example in readme if we go that way

innerRef is needed because you can't always guarantee that there will be a ref passed in from the parent (or what the shape of it is i.e. function vs RefObject). Ideally, we'd need to also support forward ref for the div as well in case the user wants that for things like focus handling.

The other option due to how basic the ResponsiveVisGraph is, is to export the hook, and put a write-up of the code to run that hook on storybook mdx file and the main readme.

Copy link
Collaborator

@WikKam WikKam left a comment

Choose a reason for hiding this comment

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

Same opinion as Grzesiek, the one with resizeObserver just looks so much better. Maybe we should keep both of them and give user the choice?

@ZachHaber
Copy link
Collaborator Author

Yup, we can definitely make both available. The one that this PR is displaying is just built into the component, so we just need to indicate to users how to turn it on.
With that I'll close this PR and get a new one with the other set of changes up shortly.

@ZachHaber ZachHaber closed this Mar 9, 2022
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.

Create separate component for responsive version of graph
3 participants