Ability to customize vega-lite charts via configuration #726

ajainarayanan · 2017-09-18T20:46:23Z

Adds the ability to pass on config to customize vegalite graphs.
References: Ability to customize vega plots rendered #721

Note:

Have not modified README about the new API. We can update once the implementation is reviewed.

ajainarayanan · 2017-09-18T20:47:14Z

@kanitw Have tried what we discussed. Let me know if I have missed something.

Ran through dev build
Ran test
Tested lib-voyager in a different project

All seemed to work the way it is expected.

kanitw

Thanks for the PR. I have some comments. Hope they make sense to you.

kanitw · 2017-09-19T04:16:57Z

src/components/plot/index.tsx

+export const Plot = connect<PlotConnectProps, {}, PlotOwnProps>(
+  (state: State) => {
+    return {
+      voyagerConfig: selectConfig(state)


Plot should be a dumb (renderer-only) component, so it should not use connect.
Instead, config should be passed from its parent component, which should already contain connect.

Let's just name it config too as config in this project (except in <VegaLite/>) should mean Voyager's config anyway.

ok makes sense if we want to extract this as a separate component (or separate repo).

kanitw · 2017-09-19T04:19:06Z

src/components/vega-lite/index.tsx

@@ -16,6 +17,8 @@ export interface VegaLiteProps {
  logger: Logger;

  data: InlineData;
+
+  vegaliteConfig: Config;


Please just name this config.

kanitw · 2017-09-19T04:21:04Z

src/index.tsx

 import { Data } from 'vega-lite/build/src/data';
 import {App} from './components/app';
 import { VOYAGER_CONFIG } from './constants';
 import { VoyagerConfig } from './models/config';
 import { configureStore } from './store';

 const store = configureStore();
-const config: VoyagerConfig = VOYAGER_CONFIG;
+let config: VoyagerConfig = VOYAGER_CONFIG;


I think the demo is a bit unnecessary (and should not be here). More importantly, the default mark color should not be "black". Please revert this file.

kanitw · 2017-09-19T04:23:32Z

src/reducers/reset.ts

@@ -13,6 +13,10 @@ export function makeResetReducer<T extends object>(
      // Need to cast as object as TS somehow doesn't know that T extends object already
      const newState = {...state as object} as T;
      Object.keys(resetIndex).forEach((key: keyof T) => {
+        // This is to not reset any property if resetIndex[property] is false.


Nice catch. Maybe add a unit test to prevent regression?

kanitw · 2017-09-19T04:28:35Z

src/lib-voyager.tsx

+ * @param {Object}          config    configuration options
+ * @param {Array}           data      data object. Can be a string or an array of objects.
+ */
+export function CreateVoyager(container: Container, data: Data, config: VoyagerConfig): Voyager {


We should not offer two versions of a function with a capital C and a lowercase C.

So you have two options:

a) Remove CreateVoyager. (Yes, this will break things, but we are currently under pre-release version, so better make things right right now. I'll note in the release note that the next release includes this breaking changes.)

b) Do not add createVoyager with the new parameter since this PR only modifies VoyagerConfig and doesn't really add new parameter to CreateVoyager anyway.

I strongly prefer a) since it's better prepared for the future. (We also previously let CreateVoyager with capital C slips in by mistake -- so better fix it before it is too late.)

If you do a), please also help:

Update README.md accordingly

submit a relevant PR for voyager-electronto update relevant API calls

@kanitw ok makes sense. I added it as a separate function for gradually letting users know that this function will be deprecated in the next release. But since voyager is still in alpha/beta releases I guess it should be ok. Its a simple API. Will revert back.

Will revert back.

Note that if you do b), we will probably still do a) in the future anyway and you'll have to update the API calls. Maybe it's better if you help us do a) now :p

👍 Will do a). Regarding PR for voyager-electron: There is an already opened PR. Not sure if I can create another one. But will see if I can spare sometime this weekend for it :)

kanitw · 2017-09-19T04:33:24Z

src/lib-voyager.tsx

@@ -36,7 +45,8 @@ export class Voyager {
  private data: Data;
  private filename: string;

-  constructor(container: Container, config: VoyagerConfig, data: Data) {
+  constructor(container: Container, params: VoyagerParams = DEFAULT_VOYAGER_PARAMS) {


Of the top of my head, I think we don't really need the default DEFAULT_VOYAGER_PARAMS as the store already has default data and config? (Let me know if I'm wrong as my memory is not perfect.)

Note that if we really need it, we probably better use

const {config=DEFAULT_VOYAGER_CONFIG, data=DEFAULT_DATA} = params;

so the default is always used if only one parameter is specified.

…plot-list and view-pane components to pass appropriate config parameter to Plot

ajainarayanan · 2017-09-20T04:24:39Z

@kanitw Have updated the PR based on review comments. Let me know if I had missed anything.

Note:

~~Haven't added unit test for the reset reducer.~~ Have added unit tests for reset reducer.

ajainarayanan · 2017-09-23T18:52:59Z

@kanitw Can you give it a try now. Have added unit tests for reset reducer.

kanitw

Thank so much for helping. I only have a few more comments.

Also, sorry the for the delay for reviewing. I'm quite busy for the next two weeks so there might be some delay.

Note that you might also want to push a few more commits to vega/voyager-electron#32 to also use the latest createVoyager command instead of the old CreateVoyager?

kanitw · 2017-09-25T04:27:53Z

src/lib-voyager.tsx

-  data: {values: ([] as any)},
-  config: DEFAULT_VOYAGER_CONFIG
-};
+const DEFAULT_DATA: any = undefined;


Remove this? (We can just not assign default data below.)

kanitw · 2017-09-25T04:28:30Z

src/lib-voyager.test.ui.tsx

@@ -163,6 +163,48 @@ describe('lib-voyager', () => {
        }
      }, 10);
    });
+    it('CreateVoyager, update config and update data should retain correct config', done => {


creates a Voyager instance, update ...

kanitw · 2017-09-25T04:29:18Z

src/reducers/reset.test.ts

+    );
+  };
+
+  it('Should reset to right default value', () => {


please use "should" (small cap)

kanitw · 2017-09-25T04:29:21Z

src/reducers/reset.test.ts

+    ).toEqual(DEFAULT_PERSISTENT_STATE);
+  });
+
+  it('Should reset bookmark when resetIndex is true', () => {


please use "should" (small cap)

kanitw · 2017-09-25T04:30:46Z

src/reducers/reset.test.ts

+    shelfPreview: true
+  };
+
+  const getResetReducer = (defaultState = DEFAULT_PERSISTENT_STATE) => {


Maybe just export persistentReducer in index.ts and use it?

Since its a unit test for reset reducer just wanted it to be self contained. If we change persistentReducer this unit test should be independent of that change as it is testing only reset reducer. May be if I change the model then it can become totally independent.

ajainarayanan · 2017-09-26T00:56:45Z

@kanitw Have updated the PR. Could you give it another try.

ajainarayanan · 2017-10-10T06:14:30Z

@kanitw Any updates on this? Does it require any more changes that I missed?

kanitw · 2017-10-12T22:05:28Z

Sorry I was on a conference and a vacation afterward so I'm a bit slow on this, but I will try to get to this soon.

kanitw

Sorry for the delay. I have a few more comments.

Please also submit a PR to update voyager-electron (perhaps fork from vega/voyager-electron#32).

kanitw · 2017-10-24T06:16:37Z

src/components/vega-lite/index.tsx

@@ -16,6 +17,8 @@ export interface VegaLiteProps {
  logger: Logger;

  data: InlineData;
+
+  config: Config;


I think config should be optional, so as logger and data (we accidentally let them slip through).

kanitw · 2017-10-24T06:17:34Z

src/components/vega-lite/index.tsx

+    vlSpec = {
+      ...vlSpec,
+      config: {
+        ...vlSpec.config,


the general config should be overridden by a spec specific-config.

{ ...config ...vlSpec.config }

Wouldn't this override the config users pass to modify vega-lite chart's configuration? My understanding was users pass vegaliteConfig to override spec configurations for respective charts (for instance modify the color or axis tick format etc.,)

Yes, it will override since we think about the shared config as a theme. So suppose you have multiple Vega-Lite charts and you have a common config that you pass in. Each spec can have config that override the common theme (rather than the other way round). :)

If that is the case would the common theme ever be applied? Say in the case of bar charts we have defined the fill color to be a blue and if users need to override that would that be possible? I am not sure if I am missing something.

Can you provide an example where a user modifies the fill color of bar charts in voyager with custom config? My assumption was the config user provides are custom config and that would override any default spec config that comes as part of voyager for vega-lite charts.

If that is the case would the common theme ever be applied?

Yes, your common theme may have 10 properties and the spec specific theme may have 1 that override the common theme so 9 properties would still be applied.

Can you provide an example where a user modifies the fill color of bar charts in voyager with custom config?

Right now there is no such case. But this is how Vega-Lite treat "common" theme too. So should CompassQL output some config in the future.

That said, thinking more about it I think I agree with you that if users explicitly specify the config, it should override what's automatically generated by CompassQL.

kanitw · 2017-10-24T06:19:20Z

src/lib-voyager.test.ui.tsx

 import {SerializableState} from './models/index';

 const DEFAULT_TIMEOUT_LENGTH = 300;

+const DEFAULT_VOYAGER_PARAMS: VoyagerParams = {


Why do we need this? (given that all of properties are declared as undefined anyway)

kanitw · 2017-10-24T07:13:55Z

src/lib-voyager.tsx

- * @param {Array}     data      data object. Can be a string or an array of objects.
+ * @param {Container}       container css selector or HTMLElement that will be the parent
+ *                                    element of the application
+ * @param {VoyagerParams}   params    Voyager params. {data, config}. Any new additional parameter


Voyager parameter object which can contain optional data and config properties.

No need to say "Any new additional parameter required by voyager can be added here as named parameter for easier visibilty" as that's not what users of Voyager would do.

ajainarayanan · 2017-10-28T21:28:38Z

@kanitw Sorry for the delayed response. Was busy at work. Have updated PR based on review comments. Can you give it a try.

ajainarayanan · 2017-11-14T07:23:32Z

Hey @kanitw . Are there any updates on this PR? Did I miss something in my last commit? Can you give another try

kanitw · 2017-11-21T04:01:03Z

Replied. Sorry for delay too, I've been quite busy and a bit sick.

kanitw · 2017-11-21T21:42:30Z

OK i'm convinced. The last part needed is to update the Voyager electron app then. :)

ajainarayanan · 2017-11-22T02:18:17Z

@kanitw Thought of updating the voyager-electron app once we have a npm release of voyager. I can try testing with local npm package but would be easier (or complete) if I work against a version of voyager from npm. Let me know if you are thinking of something else.

Note:

There is an already existing voyager-electron PR for new build changes. Might be easier if I close that and open a new PR with latest voyager changes.

kanitw · 2017-11-22T18:58:11Z

I can try testing with local npm package

I think it's better to test with local npm package first. :)

There is an already existing voyager-electron PR for new build changes. Might be easier if I close that and open a new PR with latest voyager changes.

Do you mean vega/voyager-electron#32? I guess you can fork from that PR instead of closing it -- otherwise we're losing those changes?

ajainarayanan · 2017-11-22T19:11:43Z

:) I understand. The new PR I was planning to open will have both the changes. Thats why mentioned of closing that PR. Forking a PR from an existing PR becomes a hassle to maintain. Let me see if it makes it simpler.

ajainarayanan added 4 commits September 18, 2017 13:18

Modifies voyager lib to accept vegaliteConfig as part of VoyagerConfig

ac465db

Fixes reset reducer to not reset state values whose index is false

f17ecd2

Modifies existing tests to use createVoyager api

7522924

Adds new tests for newly added createVoyager API

570eb63

kanitw requested changes Sep 19, 2017

View reviewed changes

ajainarayanan added 5 commits September 19, 2017 21:05

Fixes Plot component to accept config parameter + modifies bookmark, …

93764db

…plot-list and view-pane components to pass appropriate config parameter to Plot

Removes old API + removes demo defaults

3ceeef0

Fixes unit test based on changes to lib-voyager

4a05ca9

Adds additional unit test for makeResetReducer change

f6cb91d

Updates README

e6cf103

Adds unit test for reset reducer

9fdaca1

kanitw requested changes Sep 25, 2017

View reviewed changes

ajainarayanan added 2 commits September 25, 2017 17:47

Modifies reset reducer unit test to be independent

cf2fede

Fixes default data in lib-voyager + grammar fixes to unit tests

65b7bce

kanitw requested changes Oct 24, 2017

View reviewed changes

Fixes config parameter to be optional for createVoyager function

da97efb

kanitw approved these changes Nov 21, 2017

View reviewed changes

ajainarayanan mentioned this pull request Dec 4, 2017

Fix dependencies based voyager build change vega/voyager-electron#32

Open

ssharif6 force-pushed the master branch from 6e335ea to 3521d78 Compare May 12, 2018 04:13

Ability to customize vega-lite charts via configuration #726

Are you sure you want to change the base?

Ability to customize vega-lite charts via configuration #726

Conversation

ajainarayanan commented Sep 18, 2017

Note:

ajainarayanan commented Sep 18, 2017

kanitw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajainarayanan commented Sep 20, 2017 • edited

Note:

ajainarayanan commented Sep 23, 2017

kanitw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajainarayanan commented Sep 26, 2017

ajainarayanan commented Oct 10, 2017

kanitw commented Oct 12, 2017

kanitw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajainarayanan commented Oct 28, 2017

ajainarayanan commented Nov 14, 2017

kanitw commented Nov 21, 2017

kanitw commented Nov 21, 2017

ajainarayanan commented Nov 22, 2017

Note:

kanitw commented Nov 22, 2017

ajainarayanan commented Nov 22, 2017

ajainarayanan commented Sep 20, 2017 •

edited