Vliu link form data explore v2 #1540

vera-liu · 2016-11-03T23:29:12Z

Done:

Changed defaultFormData and defaultViz in store to match structure sent from bootstrap_data
Added onChange function to push data from form components to store
Changed some variable names from camel to snake case matching state variables in ChartContainer and ControlPanelContainer

Todo:

pass down state needed for Field components from ControlPanelContainer
datasource/viz_type should be apart from form_data and reset all form data once they are changed
add query and save button, hit right url when query button clicked

needs-review @ascott

ascott · 2016-11-04T07:56:22Z

caravel/assets/javascripts/explorev2/stores/store.js

+  x: null,
+  y: null,
+  size: null,
+  url: 'https: //www.youtube.com/embed/JkI5rg_VcQ4',


i think this was just an example url, we should make this null

ascott · 2016-11-04T07:59:09Z

caravel/assets/javascripts/explorev2/components/FieldSet.jsx

@@ -21,21 +26,41 @@ const defaultProps = {
  validators: null,
 };

-export default class FieldSet extends React.Component {
+export class FieldSet extends React.Component {
+  onChange(value) {


we could move this onChange method to ControlPanelsContainer and pass that in as a prop so we don't have to connect to stores/bind actions in this component too.

ascott · 2016-11-04T08:34:20Z

caravel/assets/javascripts/explorev2/stores/store.js

+  x_axis_format: 'smart_date',
+  y_axis_format: '.3s',
+  markup_type: 'markdown',
+  rotation: 'random',


all of these defaults should live with the other field data. there is an object called fields which contains the data each field needs to render.

rotation: { type: 'SelectField', label: 'Rotation', choices: formatSelectOptions(['random', 'flat', 'square']), default: 'random', description: 'Rotation to apply to words in the cloud', },

https://github.com/vera-liu/caravel/blob/dd3ee3ef6baf653a4dca6350abfe3f5a4bbde30b/caravel/assets/javascripts/explorev2/stores/store.js#L1453

vera-liu · 2016-11-04T18:28:52Z

Done:

Moved onChange to ChartContainer
get defaultFormData from fields in store.js

@ascott

ascott · 2016-11-07T20:23:25Z

caravel/assets/javascripts/explorev2/actions/exploreActions.js

@@ -48,18 +48,14 @@ export function setFilterColumnOpts(filterColumnOpts) {
 }

 export function resetFormData() {
-  // Clear all form data when switching datasource
+  // Clear all form data when switching datasource or viz_type
  return { type: RESET_FORM_DATA };


i'm curious why we need to reset form data when switching a datasource? the way i would see this working is when a datasource changes, it should trigger a fetch of datasource meta data which, once returned will update state with the new meta data. the meta data should include select options and any defaults needed.

When user switches datasource we would reset both choices and already selected values in all forms. I'm thinking about separating out actions for onChange of datasource/viz_type in another PR, and dispatch resetFormData and resetChoices from there. Some form_data will be set to default values ,some will be blank, e.g. metrics/columns

right, so wouldn't the data that is returned from fetching the datasource meta data be set on the state, in effect 'resetting' the form data?

right..yeah it makes sense to keep form_data used for visualization unchanged (such as show_bar_value) and only reset form_data such as columns/groupby. So I guess this is only needed for switching viz_type then? for instance when we switch from dist_bar to table, form_data such as show_control will be reset to default value false.

when a viz type changes, then the state will be updated which will trigger a rerender of the control panel sections. https://github.com/airbnb/caravel/blob/master/caravel/assets/javascripts/explorev2/components/ControlPanelsContainer.jsx#L29

the sections and fields that get rendered are dynamic depending on the viz type.

so what i am getting at is that the field defaults should be stored in the field definitions, and we shouldn't need to reset the whole form at any point, react will take care of triggering the rerender and will render the appropriate fields/default when a new viz type is updated.

ascott · 2016-11-07T20:30:03Z

caravel/assets/javascripts/explorev2/reducers/exploreReducer.js

  return newFormData;
 };

-const setVizInState = function (state, action) {
+const setVizInState = function (oldViz, action) {
  switch (action.type) {
    case actions.SET_FORM_DATA:


it looks like this only gets called SET_FORM_DATA action handler. curious why we need a switch statement to check the action type...

This is for if we will have actions for setting other attributes in viz object, such as json_endpoint, query and standalone_endpoint

i think we will want other action handlers rather than switch statements here.

…om fields in store

ascott · 2016-11-08T00:43:50Z

caravel/assets/javascripts/explorev2/reducers/exploreReducer.js

-const setFormInViz = function (state, action) {
-  const newFormData = Object.assign({}, state);
-  newFormData[action.key] = action.value;
+const setFormInViz = function (oldFd, action) {


seems like this should be called toggleCheckboxValue?

i think every method in the reducer should take state and action as args as a general pattern.

we also use this function for Select and TextField, for checkbox action.value is undefined while for other fields they are directly set to action.value. I'll add a comment to make this clear

ascott · 2016-11-08T00:47:51Z

caravel/assets/javascripts/explorev2/reducers/exploreReducer.js

-    default:
-      return state;
-  }
+const setVizInState = function (oldViz, action) {


i think we should be setting state any time any field is changed, and not just one handler for updating the whole form at once. so we should replace this a function that can set the value for a given field name.

ascott · 2016-11-08T01:21:49Z

caravel/assets/javascripts/explorev2/reducers/exploreReducer.js

-        state,
-        { viz: setVizInState(state.viz, action) }
-      );
+      const newViz = Object.assign({}, state.viz);


i think we could simplify this to

const newState = Object.assign({}, state); newState.viz.form_data[action.key] = action.value ? action.value : (!state.viz.form_data[action.key]); return newState;

ascott · 2016-11-08T01:22:56Z

caravel/assets/spec/javascripts/explore/components/actions_spec.js

-    };
-    const newState = exploreReducer(testState, actions.removeFilter(filter1));
-    expect(newState.filters).to.deep.equal([filter2]);
+  it('set form data', () => {


could we add tests to test for each field type, checkbox, select, text, etc.?

or at least for checkboxes since it's toggling the value....

ascott · 2016-11-08T01:28:31Z

caravel/assets/javascripts/explorev2/components/ControlPanelsContainer.jsx

    }
  }

+  onChange(name, value) {
+    this.props.actions.setFormData(name, value);


i might call this setFieldValue for clarity since it's just updating a single field.

vera-liu · 2016-11-08T04:21:48Z

Added tests in latest commit :) @ascott

ascott · 2016-11-08T17:55:29Z

caravel/assets/spec/javascripts/explorev2/actions_spec.js

+import { exploreReducer } from '../../../javascripts/explorev2/reducers/exploreReducer';
+
+describe('reducers', () => {
+  it('set form data according to given value', () => {


i wonder if this would be more clear as:
it('sets correct field value given a key and value')

ascott · 2016-11-08T17:56:57Z

caravel/assets/spec/javascripts/explorev2/actions_spec.js

+    const newState = exploreReducer(initialState, actions.setFieldValue('x_axis_label', 'x'));
+    expect(newState.viz.form_data.x_axis_label).to.equal('x');
+  });
+  it('flip value in state when action.key is not defined', () => {


would this be more clear as:
it('toggles a boolean field value given only a key')

ascott · 2016-11-08T17:57:52Z

LGTM! 👍 🚢

ascott reviewed Nov 4, 2016

View reviewed changes

vera-liu force-pushed the vliu_link_form_data_explore_v2 branch from aac450f to 2263027 Compare November 4, 2016 18:27

ascott reviewed Nov 7, 2016

View reviewed changes

vera-liu force-pushed the vliu_link_form_data_explore_v2 branch from bbe80d6 to 6187382 Compare November 8, 2016 00:39

vera-liu added 5 commits November 7, 2016 16:40

Put default form_data and viz in store

3b5d2ef

Link fields to store by firing setFormData action

53a6d02

Fixed tests for Container and actions

1d7c620

Moved onChange to ControlPanelsContainer, retrieve defaultFormData fr…

c56324c

…om fields in store

Deleted switch statement in reducer

8bfbd72

vera-liu force-pushed the vliu_link_form_data_explore_v2 branch from 6187382 to 8bfbd72 Compare November 8, 2016 00:40

ascott reviewed Nov 8, 2016

View reviewed changes

Removed resetFormData and refactored setFormData in reducers

7e00842

ascott reviewed Nov 8, 2016

View reviewed changes

vera-liu force-pushed the vliu_link_form_data_explore_v2 branch 2 times, most recently from abd8476 to 4424e56 Compare November 8, 2016 04:21

Added text for fields

94db0af

vera-liu force-pushed the vliu_link_form_data_explore_v2 branch from 4424e56 to 94db0af Compare November 8, 2016 16:48

ascott reviewed Nov 8, 2016

View reviewed changes

Changed test statements

1c56923

vera-liu force-pushed the vliu_link_form_data_explore_v2 branch from 0f581ff to 1c56923 Compare November 8, 2016 18:04

Merge branch 'master' into vliu_link_form_data_explore_v2

8403869

vera-liu merged commit bb6ab11 into apache:master Nov 8, 2016

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.13.0 labels Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vliu link form data explore v2 #1540

Vliu link form data explore v2 #1540

vera-liu commented Nov 3, 2016 •

edited

Loading

ascott Nov 4, 2016 •

edited

Loading

ascott Nov 4, 2016

ascott Nov 4, 2016 •

edited

Loading

ascott Nov 4, 2016

vera-liu commented Nov 4, 2016

ascott Nov 7, 2016

vera-liu Nov 7, 2016 •

edited

Loading

ascott Nov 8, 2016

vera-liu Nov 8, 2016

ascott Nov 8, 2016

ascott Nov 7, 2016

vera-liu Nov 7, 2016

ascott Nov 8, 2016

ascott Nov 8, 2016

vera-liu Nov 8, 2016 •

edited

Loading

ascott Nov 8, 2016

ascott Nov 8, 2016

ascott Nov 8, 2016

ascott Nov 8, 2016

ascott Nov 8, 2016

vera-liu commented Nov 8, 2016

ascott Nov 8, 2016 •

edited

Loading

ascott Nov 8, 2016

ascott commented Nov 8, 2016

Vliu link form data explore v2 #1540

Vliu link form data explore v2 #1540

Conversation

vera-liu commented Nov 3, 2016 • edited Loading

ascott Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascott Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vera-liu commented Nov 4, 2016

Choose a reason for hiding this comment

vera-liu Nov 7, 2016 • edited Loading

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

vera-liu Nov 8, 2016 • edited Loading

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

vera-liu commented Nov 8, 2016

ascott Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascott commented Nov 8, 2016

vera-liu commented Nov 3, 2016 •

edited

Loading

ascott Nov 4, 2016 •

edited

Loading

ascott Nov 4, 2016 •

edited

Loading

vera-liu Nov 7, 2016 •

edited

Loading

vera-liu Nov 8, 2016 •

edited

Loading

ascott Nov 8, 2016 •

edited

Loading