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

Highcarts #211

Merged
merged 12 commits into from
Dec 30, 2022
Merged

Highcarts #211

merged 12 commits into from
Dec 30, 2022

Conversation

hogmoff
Copy link
Contributor

@hogmoff hogmoff commented Dec 29, 2022

As described in issue below there are missing files for fully functional highcharts
Issue

I added the newest highcharts version 10.3.2 with all modules. But not all modules are included in chart.py because after adding all modules there where issues with nicegui.

If you are not happy about the file structure, please comment.

@falkoschindler
Copy link
Contributor

Thanks for your contribution!

Here are my thoughts about it:

  1. Currently NiceGUI does neither provide .map nor .src.js files. If we do it here, we should do it consistently for all libraries. But since the modules are more than 10MB in total, I would drop these additional files. Then there are only 1.3MB remaining.
  2. When uncommenting the remaining modules in chart.py, JavaScript complains about an "Uncaught TypeError: Cannot read properties of undefined (reading 'prototype')" in arc-diagram.js:12:348. We should either fix this error or remove the dependencies altogether.
  3. Just to make it clear: This adds 1.3MB to the default payload a NiceGUI app needs to load if "chart" is not excluded. We might need to think about some opt-in concept like the extras parameter of ui.markdown. But it's not trivial since we need to decide on an app level, not per element. In the worst case a gauge chart might be created dynamically, so all libraries must already be loaded before the chart element is even created.
  4. Since dependencies of all modules live inside the lib directory, it is indeed a bit weird to put HighCharts modules into a separate directory. But I don't immediately see a great alternative. We need to rethink this file structure as the number of dependencies grows.

In order to be able to merge this PR, we need to at least: remove unnecessary .map and .src.js files (1), include the remaining dependencies in chart.py and fix the JavaScript error (2).

Alternatively, we could remove .map and .src.js files and all dependencies that are not needed for displaying a gauge (the original problem of issue #210, resolving points 1, 2 and 3) and think about adding more HighCharts modules later.

@falkoschindler falkoschindler added the enhancement New feature or request label Dec 29, 2022
@rodja
Copy link
Member

rodja commented Dec 30, 2022

I just created branch with a sketch on how we may implement opt-in for highcharts: https://github.com/zauberzeug/nicegui/tree/highcharts-extra-deps

It could be used like this:

solid_gauge = '''
{
    chart: {
        type: 'solidgauge'
    },
    title: null,
    pane: {
        center: ['50%', '85%'],
        size: '140%',
        startAngle: -90,
        endAngle: 90,
        background: {
            backgroundColor: '#EEE',
            innerRadius: '60%',
            outerRadius: '100%',
            shape: 'arc'
        }
    },
    exporting: {
        enabled: false
    },
    tooltip: {
        enabled: false
    },
    yAxis: {
        stops: [
            [0.1, '#55BF3B'],
            [0.5, '#DDDF0D'],
            [0.9, '#DF5353'] 
        ],
        lineWidth: 0,
        tickWidth: 0,
        minorTickInterval: null,
        tickAmount: 2,
        title: {
            y: -70
        },
        labels: {
            y: 16
        },
        min: 0,
        max: 5,
        title: {
            text: 'RPM'
        }
    },
    plotOptions: {
        solidgauge: {
            dataLabels: {
                y: 5,
                borderWidth: 0,
                useHTML: true
            }
        }
    },
    series: [{
        name: 'RPM',
        data: [1],
    }]
}
'''

ui.chart(demjson3.decode(solid_gauge), ['solid-gauge'])

I'm using demjson3 to transform copy&paste gauge code into dict for ui.chart.

@hogmoff
Copy link
Contributor Author

hogmoff commented Dec 30, 2022

looks great!
Now i've although fixed the issue with some modules. The trick is to import them in a specific order.
So, we have now full functionality with low payload.

this.chart.reflow();
}, 0);
}, 1000);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not happy with the delayed loading of the chart. We should somehow make sure that the chart is loaded as soon as the extra libs are available...

@@ -19,6 +92,10 @@ def __init__(self, options: Dict) -> None:
"""
super().__init__('chart')
self._props['options'] = options
urls = [f'/_nicegui/dependencies/{id}/{dependency.path.name}'
for id, dependency in js_extra_dependencies.items()
if any(e in dependency.path.name for e in extras)]
Copy link
Contributor

Choose a reason for hiding this comment

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

@rodja Shouldn't it be

if dependency.path.name in extras

Copy link
Member

Choose a reason for hiding this comment

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

True. It could be simplified. See ee42816


class Chart(Element):

def __init__(self, options: Dict) -> None:
def __init__(self, options: Dict, extras: List[str] = []) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd mark extras as keyword argument.

Copy link
Member

Choose a reason for hiding this comment

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

Done!

@falkoschindler
Copy link
Contributor

Ok, if I understand correctly @rodja introduced a new kind of dependencies that are served like before, but only requested if an element is configured accordingly using the extras parameter. My first thoughts about it:

  • We might want to add a flag optional to the Dependency class and avoid code duplication in register_component().
  • I'd like to move the URL generation for extras from chart.py to vue.py, which is the place for managing dependencies. (dependencies.py might be a better name.)
  • I'm not sure if the generation of self._props['extras'] is correct. There might be multiple dependencies with the same path.name. In order to decide about skipping dependencies, they keep a list of their dependents, which could be useful for filtering the extras.
  • Of course, we should avoid the arbitrary delay of 1000ms before creating the chart.

@rodja Aside from this specific PR, I'd like to discuss how we could evolve the overall concept of serving and requesting dependencies. Loading dependencies via import from chart.js might be a useful pattern to load all NiceGUI dependencies only when needed.

@rodja
Copy link
Member

rodja commented Dec 30, 2022

I'll create a separate issue to generalize the dependency loading.

@rodja rodja merged commit f4e9ad7 into zauberzeug:main Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants