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

Add support for Vega and Vega-lite #3931

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ktmud
Copy link
Contributor

commented Jun 26, 2019

What type of PR is this? (check all applicable)

  • Feature

Description

Vega and Vega-Lite allow a user to specify a JSON configuration
to declaratively define a complex interactive visualization.

It could theoretically become the foundation of many visualizations
we have in Redash.

This PR adds Vega and Vega-Lite and a new visualization type,
and allows users to directly edit a Vega or Vega-Lite spec in either JSON or YAML. A lot of code is borrowed from the official Vega Editor.

While most of the functionalities are working, this is not ready for shipment yet. Some known issues:

  • Vega allows only fixed container size. We need to figure out how to do autoresize.
  • Not all themes are useful to the majority of the users.
  • Haven't tested tooltips.
  • Vega-Embed allows downloading PNG files, which can be very useful. Will add support in future PR.
  • Was trying to add monaco-yaml so the YAML editor can also have the very nice IntelliSense, including autocomplete and linting. But that Monaco plugin isn't working well with ESM yet. Will consider migrating to Monaco in future work.

Related Tickets & Documents

https://discuss.redash.io/t/add-vega-grammar-support-for-advanced-visualizations/3020

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Visualization editor:

Snip20190625_1

Visualization renderer (with a different Vega theme)

Snip20190625_3

@ktmud ktmud force-pushed the ktmud:vega branch from 697b503 to 59b2bad Jun 26, 2019

@ktmud ktmud changed the title Add support for Vega and Vega-lite [WIP] Add support for Vega and Vega-lite Jun 26, 2019

@kravets-levko

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

Hi @ktmud! Thanks for your incredible work! I know that this PR is still WIP, but I have some notices that would be better to fix on early stage:

  • don't pass query object to each visualization; you need only URLs there - so pass them; probably, data object is a good place;
  • please try to reuse existing dependencies; I like Monaco as well, but Redash already has an editor library (Ace), so please use it; also, do you really need to update all that dependencies?

Also, please see my comments to the code. Thanks!

return find(
registeredVisualizations,
viz => !viz.isDeprecated && viz.isDefault,
);

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Jun 26, 2019

Collaborator

Now this code assumes that at least one visualization has isDefault attribute; not sure if it's right or not, but I'd add a fallback for this case (if no isDefault visualization - return first non-deprecated). That's just a suggestion

This comment has been minimized.

Copy link
@ktmud

ktmud Jun 26, 2019

Author Contributor

Makes sense. Maybe we should pre-sort the registered visualizations and make the default visualization a constant instead of calling find() every time?

This is just a quick hack to avoid Vega showing up as the default editor. If there's any other way of doing it, I'd be happy to avoid adding a new config field.

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Jun 26, 2019

Collaborator

Adding new config field is a good solution, let's keep it. Calling find every time is not a problem because ATM it is used only in Edit Visualization dialog and is called there only once (on dialog open).

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Jun 30, 2019

Collaborator

@ktmud I created a separate PR for this feature (#3944) because we needed it right now in few other PRs to fix tests.

'attachment; filename="{}"'.format(filename.encode("utf-8"))
)
if filetype in ('csv', 'json') and request.args.get('download') in ['false', '0']:
response.headers.set('Content-Type', 'text/plain; charset=UTF-8')

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Jun 26, 2019

Collaborator

Can you please explain these changes (what are they needed for?). Thanks!

This comment has been minimized.

Copy link
@ktmud

ktmud Jun 26, 2019

Author Contributor

This is for allowing users to click on the data source url that populated in Vega schema and review the data directly in browser without automatically downloading a file.

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Jun 26, 2019

Collaborator

I think in this case you have to set a proper MIME type for each extension. E.g. I have a chrome extensions that pretty-print both CSV and JSON (see screenshot with example), and both that extensions detect their targets by MIME type, so changes like this will break them.

image

This comment has been minimized.

Copy link
@ktmud

ktmud Jun 30, 2019

Author Contributor

I also have a JSON prettier extension installed, but couldn't find a good one for CSV. Which one are you using?

Since this change added a new URL parameter "download=false", it shouldn't break any existing links. I'm inclined to keep it. What do you think?

@ktmud

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Hi Levko, thanks for reviewing!

  • please try to reuse existing dependencies; I like Monaco as well, but Redash already has an editor library (Ace), so please use it; also, do you really need to update all that dependencies?

Can we actually replace Ace Editor with Monaco? Monaco comes with some highly desirable advanced features such as inline validation of JSON schemas, which is quite essential for a Vega editor.

I had to upgrade Webpack because the latest version of Monaco wouldn't build.

@kravets-levko

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

Can we actually replace Ace Editor with Monaco?

Well, yes, we can, of course. But definitely not within this PR - we use a lot of Ace features in query editor and all of them should be kept with Monaco.

@ktmud

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Can we actually replace Ace Editor with Monaco?

Well, yes, we can, of course. But definitely not within this PR - we use a lot of Ace features in query editor and all of them should be kept with Monaco.

Sure! I think I'll remove Monaco dependency for now and replace it with Ace Editor. There was some building issues with Monaco as well (it takes forever to build client/dist for Docker image).

@ktmud ktmud force-pushed the ktmud:vega branch 5 times, most recently from 92e56d4 to 2e522cc Jun 30, 2019

// monaco.languages.json.jsonDefaults.setDiagnosticsOptions(monacoDiagnostics);
// monaco.languages.registerDocumentFormattingEditProvider('json', jsonFormatter);
// monaco.languages.registerDocumentFormattingEditProvider('yaml', yamlFormatter);
// }

This comment has been minimized.

Copy link
@ktmud

ktmud Jul 2, 2019

Author Contributor

Keeping the code for Monaco here for now, so later migration will be easier (if we are going to do it)

vPadding += (spec.padding.top || 0) + (spec.padding.bottom || 0);
}
width = width || specWidth || Math.max(parentSize.width - hPadding, 100);
height = height || specHeight || Math.min(400, Math.max(parentSize.height - vPadding, 300));

This comment has been minimized.

Copy link
@ktmud

ktmud Jul 2, 2019

Author Contributor

By default, a Vega visualization has min-height 300px and max-height 400px. This is inline with what Plot.ly currently has, although I was not able to find where we set the height for Plot.ly...

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Jul 2, 2019

Collaborator

We don't set height for Plotly charts: on Dashboard page chart fits it's container in widget, and on Query page it has fixed height (IIRC, 500px) and width 100%.

This comment has been minimized.

Copy link
@ktmud

ktmud Jul 2, 2019

Author Contributor

So the default height 500px comes from Plot.ly library itself?

I implemented 100% height for Vega charts everywhere.

@ktmud ktmud changed the title [WIP] Add support for Vega and Vega-lite Add support for Vega and Vega-lite Jul 2, 2019

@ktmud ktmud force-pushed the ktmud:vega branch from 4dcc779 to 5a22339 Jul 2, 2019

@ktmud

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@kravets-levko @arikfr I think this PR is more or less ready now. Can you please take another look? It's actually quite fun to play with Vega specs.

import { Handler } from 'vega-tooltip';
import { Alert, Icon } from 'antd';
import LZString from 'lz-string';
import memoize from 'memoize-one';

This comment has been minimized.

Copy link
@kravets-levko

This comment has been minimized.

Copy link
@ktmud

ktmud Jul 2, 2019

Author Contributor

_.memoize does not cache only one return value, having higher risk of memory leaks.
useMemo does not work with class components. I don't see the added benefits of rewriting my class to a function component.

memoize-one only has 45 LOC.

@ktmud ktmud referenced this pull request Jul 8, 2019

Closed

Vegalite charts #4558

ktmud added some commits Jun 26, 2019

Add support for Vega and Vega-lite
Vega and Vega-Lite allow users to specify a JSON configuration
to declaratively define a complex interactive visualization.

It could thoeretically become the foundation of many visualizations
we already have.

This commit adds integration with Vega and Vega-Lite, with a lot
of code borrowed from the official Vega Editor[1].

[1] https://vega.github.io/editor/
Apply vega theme AFTER Vega-Lite -> Vega compile
and update data source BEFORE compile.

@ktmud ktmud force-pushed the ktmud:vega branch from da21310 to c3394e4 Jul 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.