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

data/json-connector #19560

Merged
merged 22 commits into from
Aug 31, 2023
Merged

data/json-connector #19560

merged 22 commits into from
Aug 31, 2023

Conversation

pawellysy
Copy link
Member

@pawellysy pawellysy commented Aug 16, 2023

Implemented the JSON Connector for the Dashboards.

@pawellysy pawellysy self-assigned this Aug 16, 2023
@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Aug 16, 2023

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

master candidate difference
dashboards/dashboards.js 42.7 kB
146.3 kB
43.0 kB
148.3 kB
332 B
2066 B

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Aug 16, 2023

Visual test results - No difference found


Samples changed

Change type Sample
Added samples/highcharts/data-tools/datapool-json-connector-orientation/demo.js
Modified samples/highcharts/data-tools/datapool-json-connector/demo.js
Modified samples/highcharts/data-tools/datapool-json-connector/demo.js
Added samples/highcharts/data-tools/datapool-json-connector/demo.js

@pawellysy pawellysy changed the title Initial implementation of JSON connector. data/json-connector Aug 16, 2023
@bre1470
Copy link
Contributor

bre1470 commented Aug 16, 2023

Looks good. Thinks that might be considered:

  • default to row orientation like in Highcharts
  • default to auto column names like in spreadsheets -> A, B, C,
  • maybe support of a columnNames or keys option like in Highcharts
  • support for array of row objects would be nice as well
  • removing unused liveDataURL and liveDataTimeout

@karolkolodziej karolkolodziej marked this pull request as ready for review August 29, 2023 06:41
Copy link
Contributor

@karolkolodziej karolkolodziej left a comment

Choose a reason for hiding this comment

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

Make sure the API documentation is generated properly. Add JSON here: https://api.highcharts.com/dashboards/#interfaces/Data_DataPoolOptions.DataPoolOptions-1

docs/dashboards/data-handling.md Outdated Show resolved Hide resolved
ts/Data/Connectors/JSONConnector.ts Outdated Show resolved Hide resolved
ts/Data/Connectors/JSONConnectorOptions.d.ts Show resolved Hide resolved
ts/Data/Connectors/JSONConnectorOptions.d.ts Outdated Show resolved Hide resolved
ts/Data/Connectors/JSONConnectorOptions.d.ts Outdated Show resolved Hide resolved
ts/Data/Converters/JSONConverter.ts Outdated Show resolved Hide resolved
ts/Data/Converters/JSONConverter.ts Outdated Show resolved Hide resolved
ts/Data/Converters/JSONConverter.ts Outdated Show resolved Hide resolved
ts/Data/Converters/JSONConverter.ts Outdated Show resolved Hide resolved
ts/Data/Converters/JSONConverter.ts Outdated Show resolved Hide resolved
pawellysy and others added 7 commits August 29, 2023 09:29
Co-authored-by: Karol Kołodziej <57004120+karolkolodziej@users.noreply.github.com>
Co-authored-by: Karol Kołodziej <57004120+karolkolodziej@users.noreply.github.com>
Co-authored-by: Karol Kołodziej <57004120+karolkolodziej@users.noreply.github.com>
Co-authored-by: Karol Kołodziej <57004120+karolkolodziej@users.noreply.github.com>
Co-authored-by: Karol Kołodziej <57004120+karolkolodziej@users.noreply.github.com>
Co-authored-by: Karol Kołodziej <57004120+karolkolodziej@users.noreply.github.com>
Copy link
Contributor

@sebastianbochan sebastianbochan left a comment

Choose a reason for hiding this comment

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

Can we add a demo, where we load the JSON from the URL?

ts/Data/Connectors/JSONConnector.ts Show resolved Hide resolved
ts/Data/Converters/JSONConverter.ts Show resolved Hide resolved
ts/Data/Converters/JSONConverter.ts Outdated Show resolved Hide resolved
ts/Data/Converters/JSONConverter.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@bre1470 bre1470 left a comment

Choose a reason for hiding this comment

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

LGTM

@bre1470
Copy link
Contributor

bre1470 commented Aug 30, 2023

Nice work!

@pawellysy pawellysy merged commit 5cce605 into master Aug 31, 2023
6 of 11 checks passed
@highsoft-bot highsoft-bot added this to the Dashboards-Next milestone Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants