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:Responsive support for echart #273

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sanuki-073
Copy link

Responsive support in solara/component/echarts.py and solara/component/echarts.vue

GIF of the actual resiseable grid layout in action can be found in following issue.
#268

Copy link
Contributor

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Hi,

thanks a lot for opening this PR!
I think it's okay that it does not have an explicit example.
I wonder if responsive should be the default, and why someone would want to disable it (@mariobuikhuizen what do you think).

There are a lot of unrelated changes to this PR, which I would like you to revert. I don't agree with the other changes and those may drag this PR on for a long time. I suggest you revert them and open new issues, or a PR for the other plans you have with this component. This way we can get this PR merged soon, and not get distracted by the other changes.
What do you think?

Regards,

Maarten

@@ -10,7 +10,8 @@ class EchartsWidget(ipyvuetify.VuetifyTemplate):
template_file = (__file__, "echarts.vue")

attributes = traitlets.Dict(default_value=None, allow_none=True).tag(sync=True)

parent_attributes = traitlets.Dict(default_value=None, allow_none=True).tag(sync=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parent_attributes = traitlets.Dict(default_value=None, allow_none=True).tag(sync=True)

Comment on lines +45 to +46
attributes={"style": "width:100%;height:100%;min-height:400px"},
parent_attributes={"style": "width: 100%; height: 100%; overflow: hidden;"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
attributes={"style": "width:100%;height:100%;min-height:400px"},
parent_attributes={"style": "width: 100%; height: 100%; overflow: hidden;"},
attributes={"style": "height: 400px"},

@@ -60,6 +63,8 @@ def FigureEcharts(
* on_mouseout: Callable, a function that will be called when the user moves the mouse out of a certain component.
* maps: dict, a dictionary of maps to be used in the figure.
* attributes: dict, a dictionary of attributes to be passed to the container (like style, class).
* parent_attributes: dict, a dictionary of attributes to be passed to the parent container (like style, class).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* parent_attributes: dict, a dictionary of attributes to be passed to the parent container (like style, class).

@@ -60,6 +63,8 @@ def FigureEcharts(
* on_mouseout: Callable, a function that will be called when the user moves the mouse out of a certain component.
* maps: dict, a dictionary of maps to be used in the figure.
* attributes: dict, a dictionary of attributes to be passed to the container (like style, class).
* parent_attributes: dict, a dictionary of attributes to be passed to the parent container (like style, class).
* responsive: bool, whether the figure should be responsive or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* responsive: bool, whether the figure should be responsive or not.
* responsive: bool, whether the chart should resize when the container changes size.

@@ -72,4 +77,6 @@ def FigureEcharts(
on_mouseout=on_mouseout,
on_mouseout_enabled=on_mouseout is not None,
attributes=attributes,
parent_attributes=parent_attributes,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parent_attributes=parent_attributes,

@@ -1,5 +1,5 @@
<template>
<div>
<div class="solara-box" v-bind="parent_attributes">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div class="solara-box" v-bind="parent_attributes">
<div>

Comment on lines +17 to +34
console.log("responsive:"+str(this.responsive));
if(this.responsive){
this.resizeObserver = new ResizeObserver(entries => {
for (let entry of entries) {
if (entry.target === this.$refs.echarts) {
this.handleResize();
}
}
});
this.resizeObserver.observe(this.$refs.echarts);
};
},
beforeDestroy() {
if (this.resizeObserver) {
this.resizeObserver.unobserve(this.$refs.echarts);
this.resizeObserver.disconnect();
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the log and format/indent the code similar to the rest of the document style.

Comment on lines +86 to +90
handleResize() {
if (this.chart) {
this.chart.resize();
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
handleResize() {
if (this.chart) {
this.chart.resize();
}
},
handleResize() {
if (this.chart) {
this.chart.resize();
}
},

I think handleContainerResize is a better name, what do you think? Could you please indent the code?

Comment on lines +149 to +152

}
.solara-box {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
.solara-box {

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.

None yet

2 participants