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

Upgrade ECharts to v5, add support for ECharts gauge & chart widget #814

Merged
merged 8 commits into from
Jan 27, 2021

Conversation

crnjan
Copy link
Contributor

@crnjan crnjan commented Jan 16, 2021

Based on feedback prepared a PR - implemented support for gauge. Adding it to chart page example:

config:
  label: Test Gauge
  sidebar: true
slots:
  series:
    - component: oh-data-series
      config:
        type: gauge
        data:
          - value: =Number.parseFloat(items.gBasement_BoilerRoom_HP_RAD_Return.state)
            name: HEATING
        detail:
          formatter: "{value} °C"
        axisLine:
          lineStyle:
            width: 8
            color:
              - - =Number.parseFloat(items.gBasement_BoilerRoom_HP_RAD_MinimalRadiatorReturn.state)
                  / 100
                - "#58D9F9"
              - - =Number.parseFloat(items.gBasement_BoilerRoom_HP_RAD_MaximalRadiatorReturn.state)
                  / 100
                - "#7CFFB2"
              - - 1
                - "#FF6E76"

renders

Screenshot 2021-01-16 at 21 03 15

Also added a widget wrapper for oh-chart-page so one can use all available ECharts goodies in layout pages, i.e.

config:
  label: Test Gauge
  sidebar: true
blocks: []
masonry:
  - component: oh-masonry
    slots:
      default:
        - component: oh-chart
          config: {}
          slots:
            series:
              - component: oh-data-series
                config:
                  type: gauge
                  data:
                    - value: 42

renders

Screenshot 2021-01-16 at 21 39 21

A bit heavier example with gauges, line chart and some other cards ...

Screenshot 2021-01-16 at 20 44 34

There are still some pending tasks, as adding ability to hide period controls, ... but would like first to get feedback if going into the right direction.

I look forward to any feedback or comments.

Signed-off-by: Boris Krivonog [email protected]

@crnjan crnjan requested a review from a team as a code owner January 16, 2021 20:54
@ghys
Copy link
Member

ghys commented Jan 17, 2021

Love the result, some initial thoughts on the implementation:

  • not sure having a widget embedding a oh-chart-page is the best idea logically-wise, perhaps it should be the other way around;
  • widgets are part of the app's webpack "entry point", having it imported like this would mean the entire ECharts library will be included and loaded on startup, which isn't right.

Have a look at this: #648 (comment)

And execute npm run webpack-analyzer, wait a few minutes, and verify in the page that opens that ECharts is still in its own separate bundle, and not part of the main app.js:

image

@crnjan
Copy link
Contributor Author

crnjan commented Jan 18, 2021

not sure having a widget embedding a oh-chart-page

That was my impression too, but wanted to start with a minimal set of changes first so if going into the wrong direction, would not be going too far ;)

Thanks for the lazy loading info, now ECharts are back in their own bundle.

@crnjan
Copy link
Contributor Author

crnjan commented Jan 18, 2021

Gauges look better without the period control ;)

Screenshot 2021-01-18 at 14 47 38

Base automatically changed from master to main January 18, 2021 20:05
@crnjan
Copy link
Contributor Author

crnjan commented Jan 18, 2021

Chart widget is now embedded into chart page, where styles might not be the cleanest (it's been years since I did CSS) but at least after initial testing seems as its working ... of course more testing is required.

The newly introduced oh-data-series uses no special/more convenient additional items, besides the ones specified by the ECharts gauges itself - so it's merely 1:1 mapping from yaml, expressions are evaluated only for

  • root object itself,
  • data array and
  • axisLine.lineStyle.color array.

Compared to other widgets, there is usually an item property, but not 100% sure how to fit it here. I like the current "raw" approach since it's very easy to map between ECharts documentation and samples (1:1) but on the other hand it might be more difficult for OH users to use, i.e.

    - component: oh-data-series
      config:
        type: gauge
        data:
          - value: =Number.parseFloat(items.gBasement_BoilerRoom_HP_RAD_Return.state)
            name: HEATING

vs.

    - component: oh-data-series
      config:
        type: gauge
        data:
          - item: gBasement_BoilerRoom_HP_RAD_Return

Another example for controlling the gauge's scale, ECharts syntax is:

axisLine: {
    lineStyle: {
        width: 6,
        color: [
            [0.25, '#FF6E76'],
            [0.5, '#FDDD60'],
            [0.75, '#58D9F9'],
            [1, '#7CFFB2']
        ]
    }
}

so mapped to yaml example (with dynamic ranges) would be something like:

axisLine:
    lineStyle:
    width: 8
    color:
        - - =Number.parseFloat(items.gBasement_BoilerRoom_HP_RAD_MinimalRadiatorReturn.state) / 100
        - "#58D9F9"
        - - =Number.parseFloat(items.gBasement_BoilerRoom_HP_RAD_MaximalRadiatorReturn.state) / 100
        - "#7CFFB2"
        - - 1
        - "#FF6E76"

While it works perfectly fine, syntax might feel a bit odd.

Comments and suggestions welcome and appreciated.

Did some testing with chart pages, tabs, layouts, ... testing if existing functionality still works + new features ... since there are plenty of areas that might be affected, if there is any brave sole who would be willing to test gauges and chart widget (and everything else :) ), please download the ui bundle from here or install directly using

bundle:update org.openhab.ui https://github.com/crnjan/openhab-webui/raw/feature/echarts_gauge_updates/bundles/org.openhab.ui/bin/org.openhab.ui-3.1.0-SNAPSHOT.jar

Bundle contains latest main branch (at the time of writing) + changes included in this PR.

Worth mentioning is that it seems gauges don't play super nice with dark theme (probably can be solved with setting appropriated colours in yaml) but AFAIK dark mode has better support in ECharts ver 5 so this will probably improve when we migrate to latest version of ECharts.

Please report back any issues and/or comments.

Disclamer: please also be aware that PR might change and so will the YAML structure&format - meaning you might need to re-do at least parts of things you do with the ui bundle in its current state.

@crnjan
Copy link
Contributor Author

crnjan commented Jan 19, 2021

Updated package.json to

    "echarts": "^5.0.1",

and tested a bit - seems as it works out of the box, did not find any issue so far, working with gauges is easier and more powerfull

Screenshot 2021-01-19 at 14 02 19

charts seem more colorfull by default too

Screenshot 2021-01-19 at 14 06 59

dark mode plays better too

Screenshot 2021-01-19 at 14 26 13

If someone wants to test Main UI with ECharts 5, please run

bundle:update org.openhab.ui https://github.com/crnjan/openhab-webui/raw/feature/echarts_gauge_updates/bundles/org.openhab.ui/bin/org.openhab.ui-3.1.0-SNAPSHOT_echarts_5.jar

Please report back any issues - thank you!

@ghys
Copy link
Member

ghys commented Jan 19, 2021

Excellent!

dark mode plays better too

that blueish background looks a bit weird though, we'll probably have to override it.

@crnjan remove the [WIP] in the title when you're ready and I'll have a look.

@crnjan
Copy link
Contributor Author

crnjan commented Jan 19, 2021

Blueish background comes with ECharts as it seems, will try to get rid of it - that said, does it make sense for you if I include the 5.0.1 ECharts too in my PR?

@ghys
Copy link
Member

ghys commented Jan 19, 2021

Yes there were some problems with the calendar and others when in dark mode so I added a bunch of code to override the default unless there's specified by the user in the configuration, for example:

if (document && document.documentElement.classList.contains('theme-dark')) {

does it make sense for you if I include the 5.0.1 ECharts too in my PR?

Definitely. We'll have to check everything but migration looks like smooth sailing.
It's out of the question for the upcoming 3.0.1 stable patch release anyways, but we can have it planned for a 3.1.0.M1 milestone.

@crnjan crnjan force-pushed the feature/echarts_gauge branch from f9603bb to ab9e087 Compare January 19, 2021 14:51
@crnjan crnjan changed the title [WIP] Add support for ECharts gauge and chart widget Add support for ECharts gauge and chart widget Jan 19, 2021
@crnjan
Copy link
Contributor Author

crnjan commented Jan 19, 2021

Removed blueish background, same page as in my previous posts, but now from my phone with dark mode

IMG_182956F51049-1

Only thing I noticed is a console log

"import echarts from 'echarts/lib/echarts'" is not supported anymore. Use "import * as echarts from 'echarts/lib/echarts'" instead;

While all seems to work regardless, changing

import echarts from 'echarts/lib/echarts'

to

import * as echarts from 'echarts/lib/echarts'

in 'ECharts.vue' fixes this - not sure what is the best way to approach this - ignore until it's fixed in vue-echarts, fork and fix, ... ?

Also removed the WIP and looking forward to feedback.

@ghys
Copy link
Member

ghys commented Jan 21, 2021

Thanks for taking care of this.

"import echarts from 'echarts/lib/echarts'" is not supported anymore. Use "import * as echarts from 'echarts/lib/echarts'" instead;

That's unfortunate, we can't leave that warning there. and we can't really change code in dependencies. Forking is an option, but seeing there's already demand for it ecomfe/vue-echarts#491, I can't imagine it being a problem for very long, either it will be solved or a fork will appear and we'll just use that.

Thanks for your efforts, I'll do a proper review soon!

@crnjan crnjan force-pushed the feature/echarts_gauge branch from 194762e to 9693781 Compare January 23, 2021 10:24
@ghys
Copy link
Member

ghys commented Jan 24, 2021

@crnjan I have tested https://github.com/ambit-tsai/echarts-for-vue as a replacement of vue-echarts, it does work and removes the error in the console, but introduces some bugs as well...

@crnjan
Copy link
Contributor Author

crnjan commented Jan 25, 2021

I've seen https://github.com/ambit-tsai/echarts-for-vue but seems as vue-echarts is much more known/used - i.e.

vue-echarts - 25k+ weekly downloads vs echarts-for-vue - 300 weekly dowloads ...

However, it does seem as the later is more active while vue-echarts doesn't have a single commit for half a year now ...

Nevertheless, I forked the vue-echarts and created a patched branch I use for testing - only import part is changed - commit.

Updated reference to:

"vue-echarts": "github:crnjan/vue-echarts#a8af53b",

and been playing a bit last couple of days, i.e. hosting charts on overview screen (if someone sees a need for it :) ):

Screenshot 2021-01-25 at 22 01 08

While it seems to fit my use-cases, I guess more testing would be welcome ...

@ghys
Copy link
Member

ghys commented Jan 25, 2021

You're right about vue-echarts being the more popular option, but it indeed looks abandoned. I noticed the warning (error) message about the import only appears in development, not in production, so I'd be willing to not consider it a blocking issue anymore for this PR, and either ignore it (since it will not be shown to users' consoles) and eventually upgrade vue-echarts when the fix is integrated, or use your fork.
I've been playing a little with the PR and actually have it on my production system at the moment, and don't seem to notice any bugs introduced by ECharts 5 if we keep using vue-echarts. (I noticed a couple warnings about deprecations though).
So I think we definitely can schedule it for 3.1.0.M1.

Fix some linting errors, remove param group for page,
widget definition for oh-chart-component, background
color in z-wave network chart, OhChartPageDefinition in
YAML hints for chart pages.

Signed-off-by: Yannick Schaus <[email protected]>
Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

I made a few fixes regarding linting - having #833 will ensure further PRs won't have these problems because it will not build if there are linting errors - and a few other things here and there, but overall, LGTM.
The fact that the new features are only available in YAML is probably something that we'll have to improve, like:

  • a gauge card widget in the standard library (or integrate the EChart gauge to the existing gauge card), or a chart card;
  • an ability to add gauges in the chart page designer (like grids & calendars).

Many thanks @crnjan for your work!

(the queue in Jenkins is full and the PR build will not be made tonight, I'll merge tomorrow if it succeeds).

@ghys ghys added enhancement New feature or request main ui Main UI labels Jan 25, 2021
@ghys ghys added this to the 3.1 milestone Jan 25, 2021
@ghys ghys changed the title Add support for ECharts gauge and chart widget Upgrade ECharts to v5, add support for ECharts gauge & chart widget Jan 27, 2021
@ghys ghys merged commit 379cbbe into openhab:main Jan 27, 2021
@crnjan
Copy link
Contributor Author

crnjan commented Jan 27, 2021

Thank you @ghys for your guidance and all the hard work!

bigbasec pushed a commit to bigbasec/openhab-webui that referenced this pull request Jan 28, 2021
…penhab#814)

Add a oh-chart widget to add charts to layout pages; change oh-chart-page to use it.
Add a oh-data-series component with the ability to add gauges representing a single value.
Upgrade ECharts to v5.0.1.

Also-by: Yannick Schaus <[email protected]>
Signed-off-by: Boris Krivonog <[email protected]>
Signed-off-by: Brian Homeyer <[email protected]>
bigbasec pushed a commit to bigbasec/openhab-webui that referenced this pull request Jan 28, 2021
…penhab#814)

Add a oh-chart widget to add charts to layout pages; change oh-chart-page to use it.
Add a oh-data-series component with the ability to add gauges representing a single value.
Upgrade ECharts to v5.0.1.

Also-by: Yannick Schaus <[email protected]>
Signed-off-by: Boris Krivonog <[email protected]>
Signed-off-by: Brian Homeyer <[email protected]>
@crnjan crnjan deleted the feature/echarts_gauge branch February 7, 2021 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants