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

EChart width on initial load #1563

Closed
thetableman opened this issue Sep 6, 2023 · 12 comments · Fixed by #1570
Closed

EChart width on initial load #1563

thetableman opened this issue Sep 6, 2023 · 12 comments · Fixed by #1570
Labels
help wanted Extra attention is needed
Milestone

Comments

@thetableman
Copy link
Contributor

thetableman commented Sep 6, 2023

Description

Consider the below minimal example of an EChart nested within a ui.expansion:

from nicegui import ui
from random import random

@ui.refreshable
def chart():
    series_1 = [random(),random()]
    series_2 = [random(),random()]

    echart = ui.echart({
        'xAxis': {'type': 'value'},
        'yAxis': {'type': 'category', 'data': ['A', 'B'], 'inverse': True},
        'legend': {'textStyle': {'color': 'gray'}},
        'series': [
            {'type': 'bar', 'name': 'Alpha', 'data': series_1},
            {'type': 'bar', 'name': 'Beta', 'data': series_2},
        ],
    }).classes('w-full')

with ui.expansion('Expansion').classes('w-full bg-primary text-white'):
    with ui.card().classes('w-full'):
        with ui.card().classes('w-full'):
            with ui.row():
                ui.button('Update', on_click=lambda: chart.refresh())
            with ui.row().classes('w-full'):
                chart()
ui.run()

The element doesn't show full width as expected but instead takes on a compact arbitrary width, after clicking the Update button the chart refreshes and shows at the expected width. The cause seems to be the ui.expansion element as when this is removed the issue doesn't occur.

On initial load:
265870059-783c9bdc-fed6-47af-8965-dfa2109449a2

After clicking Update:
265870116-dc73905e-acba-4c78-9bda-71f63e33d761

@rodja
Copy link
Member

rodja commented Sep 6, 2023

Yes, there is something wrong with echart initial loading. I could further boil down the code to reproduce it:

from nicegui import ui, Client
import random

@ui.page('/')
async def index(client: Client):
    await client.connected()
    with ui.card().classes('w-full bg-primary text-white'):
        series = [random.random(), random.random()]

        ui.echart({
            'xAxis': {'type': 'value'},
            'yAxis': {'type': 'category', 'data': ['A', 'B'], 'inverse': True},
            'legend': {'textStyle': {'color': 'gray'}},
            'series': [{'type': 'bar', 'name': 'Data', 'data': series}, ],
        }).classes('w-full bg-white')

ui.run()

Without the await client.connected() the chart is not rendered at all. But if you then also remove with ui.card().classes('w-full bg-primary text-white'): it's working again.

@rodja rodja added this to the 1.3.14 milestone Sep 6, 2023
@rodja rodja added the help wanted Extra attention is needed label Sep 6, 2023
@rodja
Copy link
Member

rodja commented Sep 6, 2023

Maybe @natankeddem or someone else can have a look into this?

@natankeddem
Copy link
Contributor

I should have time to look into this tomorrow.

@natankeddem
Copy link
Contributor

So from some searching it appears like echarts doesn't like the fact that when closed; the expansion has the display:none style. Doing a google search for echarts display: none brings up quite a few people having a similar issue in various places.

Issuing a this.chart.resize() once the expansion becomes visible is enough to render the chart correctly. I don't know if you guys have had to deal with similar issues in other elements in the past? I will need to do more learning/research on how best to handle this issue.

@natankeddem
Copy link
Contributor

So a few initial ideas:

  • Set explicit height/width.
echart = ui.echart({
    'xAxis': {'type': 'value'},
    'yAxis': {'type': 'category', 'data': ['A', 'B'], 'inverse': True},
    'legend': {'textStyle': {'color': 'gray'}},
    'series': [
        {'type': 'bar', 'name': 'Alpha', 'data': series_1},
        {'type': 'bar', 'name': 'Beta', 'data': series_2},
    ],
}).style('height: 600px').style('width: 600px')
  • Use the show event from the expansion to refresh the chart.
ui.expansion('Expansion').classes('w-full bg-primary text-white').on('show', lambda: chart.refresh())

@rodja Do you think either of these are acceptable? I could also put in a PR for a utility method resize which would force resize the ECharts object without changing anything else.

@thetableman
Copy link
Contributor Author

Thanks for looking into this @natankeddem! I saw there was a resize PR from a user called 'dabenny' - is that also you?

I think these are both perfectly reasonable work-arounds for specific cases like the examples discussed (I'm leaning towards option 2) but there is a risk that this issue will continue to be raised by other users as the behavior is not in-keeping with other elements.

@natankeddem
Copy link
Contributor

That is not me. Do people typically have multiple Github accounts?

@rodja
Copy link
Member

rodja commented Sep 7, 2023

@natankeddem Thanks for analyzing the issue and proposing the two solutions. Option 1 is suboptimal in my view because it does not allow simple dynamic sizing. But I like Option 2 and wonder if we could simply add self.on('show', lambda: chart.refresh()) to the init function of ui.echart. The pull request #1570 from @dabenny would require people to know about the bug and manually add the logic...

@natankeddem
Copy link
Contributor

natankeddem commented Sep 7, 2023

@rodja in that case the init would either need

  • to pass in the expansion as an argument
  • dynamically search the slot tree for any expansions as parent/great grandparent/ great great grandparent...ect

The self event is tied to expansion elements not the echart. Then either way you would want a resize method to call as it just resizes and does nothing else to the chart.

@natankeddem
Copy link
Contributor

The other idea which I am not sure how to implement yet is that we could have an event that fires in the javascript that would resize the chart on visibility change. I would need to research how this could be accomplished and if it is possible at all.

@natankeddem
Copy link
Contributor

I have given @dabenny my recommendations in #1570 for an "automatic" resize event driven approach. This seems to be working in my preliminary testing.

@falkoschindler
Copy link
Contributor

I also think that an automatic resizing is the way to go. I'll look into #1570 shortly.

@falkoschindler falkoschindler linked a pull request Sep 15, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants