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

Explore: resize graph on window resize #13529

Merged
merged 2 commits into from
Oct 5, 2018
Merged

Conversation

davkal
Copy link
Contributor

@davkal davkal commented Oct 4, 2018

Fixes #13517

@torkelo
Copy link
Member

torkelo commented Oct 4, 2018

The react dashboard grid uses a sizeMe lib, not sure that could be useful in this scenario

@davkal
Copy link
Contributor Author

davkal commented Oct 4, 2018

I find the libs quite bloated. Some are useful because they polyfill the window object needed for SSR. But we'd need to revisit the whole code anyway when introducing that.

@torkelo
Copy link
Member

torkelo commented Oct 4, 2018

I find the libs quite bloated.

sizeMe is bloated? In what way? looks very small an lean and isolates this interaction so much more cleanly. A lot better than manually hooking up events to window directly from the component and having to unsub on component did unmount.

Also it handles cases that this solution does not, like hiding the sidebar (will not cause a resize event) but the available space for graph will increase.

@torkelo
Copy link
Member

torkelo commented Oct 4, 2018

I mean the Graph could be simplified by wrapping it in sizeMe HOC that adds width, then you dont need to use jquery to calculate width in the draw function.

@davkal
Copy link
Contributor Author

davkal commented Oct 5, 2018

Good points, I'll give it a try.

@@ -68,7 +69,7 @@ const FLOT_OPTIONS = {
// },
};

class Graph extends Component<any, any> {
class Graph extends Component<any, { showAllTimeSeries: boolean }> {
Copy link
Contributor

Choose a reason for hiding this comment

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

extract to interface?

interface State {
 showAllTimeSeries: boolean;
}

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

nice!

@davkal davkal merged commit f6d3325 into master Oct 5, 2018
@davkal davkal deleted the davkal/13517-resize-graph branch October 5, 2018 14:14
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