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

Make EUIXYChart responsive using ResizeObserver #1041

Merged
merged 7 commits into from
Jul 30, 2018
Merged

Make EUIXYChart responsive using ResizeObserver #1041

merged 7 commits into from
Jul 30, 2018

Conversation

markov00
Copy link
Member

This drops the use of native react-vis makeVisFlexible HOC in favour of the ResizeObserver to support resizing of the chart container as in kibana dashboard.

It also fix some resizing problem in flexbox discrovered on the new editor POC by @cchaos.

width: 0,
};
this.containerRef = React.createRef();
this.ro = new ResizeObserver(this.createResizeObserver);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather call this onResize since it's not really a factory that you call that would create anything.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

This seems to be working a lot better, ty! Just a couple quick notes mostly about the doc example.

CHANGELOG.md Outdated
- To make it more accessible, added a random id to `EuiSwitch`'s id prop if none is passed. ([#779](https://github.com/elastic/eui/pull/779))
- Fixed `EuiXYChart` wrong responsive resize in a flexbox layout ([#1041](https://github.com/elastic/eui/pull/1041))
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the word 'wrong'

},
],
demo: (
<div style={{ margin: 60 }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need the extra margin here. Let it fill the width of the example area.

text: (
<div>
<p>
You can omit <EuiCode>width</EuiCode> ando/or <EuiCode>height</EuiCode>
Copy link
Contributor

Choose a reason for hiding this comment

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

extra "o" in ando/or

You can omit <EuiCode>width</EuiCode> ando/or <EuiCode>height</EuiCode>
prop and the chart takes the full width and/or height of it&apos;s parent.
</p>
<p>The parent container needs to have computed a height and or width.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

add "/" after "and" to be "and/or"

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

nice solution, LGTM!

@cchaos
Copy link
Contributor

cchaos commented Jul 26, 2018

@markov00 I think this is safe to merge. I have been playing around with it in the prototype and it's been working much nicer.

@snide
Copy link
Contributor

snide commented Jul 27, 2018

@markov00 Can you rebase and merge this when you have time. It's holding up @cjcenizal's rename PR #1050 which would be nice to get in.

@markov00 markov00 merged commit 22238ad into elastic:master Jul 30, 2018
@markov00 markov00 deleted the fix/chart-resize branch July 30, 2018 09:59
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.

5 participants