Put formData in store #1281

vera-liu · 2016-10-07T02:10:19Z

Done:

added viz, formData into store with same structure as bootstrapData.viz and form_data
rewrite reducers and action creators for new structured store
Added SelectArray component
Todo:
do this for other vizTypes
rewrite tests for reducers

will merge this first and modify PRs for other viz types

needs-review @ascott

bkyryliuk · 2016-10-07T16:21:18Z

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

    },
    [actions.SET_HAVING_CLAUSE]() {
-      return Object.assign({}, state, { havingClause: action.havingClause });
+      return {


looks like a lot of code repitition, what about defining the function setFormData that will modify the state for the form data
return setFormData({having: action.having})

bkyryliuk · 2016-10-07T16:22:57Z

caravel/assets/javascripts/explorev2/index.jsx

-  whereClause: bootstrapData.viz.form_data.where,
+  viz: {
+    formData: {
+      sliceId: bootstrapData.viz.form_data.slice_id,


I like @mistercrunch approach to the
formData=bootstrapData and make sure that bootstrapData returned from the server complies to the UI expectations

This part is for transferring variable names from snake_case to camelCase for javascript conventions. We haven't concluded the best way for this yet. Any suggestions?

@bkyryliuk what do you mean about making sure the data complies to the UI expectations? can you clarify?

it's fine to format the json output in the camel case on python side.

bkyryliuk · 2016-10-07T16:23:35Z

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

+      where: null,
+      columns: [],
+      orderings: [],
+      rowLimit: 50000,


it would be nice to move this const to the backend - into the caravel config

Thanks for bringing it up! I will issue a separate PR for setting all the default values in store based on what we have in forms.py :)

bkyryliuk · 2016-10-07T16:23:53Z

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

+      columns: [],
+      orderings: [],
+      rowLimit: 50000,
+      timeStampFormat: 'smart_date',


bkyryliuk · 2016-10-07T16:25:01Z

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

  searchBox: false,
  whereClause: '',
  havingClause: '',
  filters: [],
  filterColumnOpts: [],
+  viz: {
+    formData: {


could you do defaultFormData: defaultFormData ?

ascott · 2016-10-07T18:29:05Z

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

-          </div>
-        </div>
+const GroupBy = (props) => {
+  const selects = [];


we can assign these select objects directly, rather than using .push

also, we could make use of the spread operator here for the common keys for each select.

const commonOpts = { multi: true, width: '12', }; const selects = [ { key: 'groupByColumns', title: 'Group By', options: props.groupByColumnOpts, value: props.groupByColumns, ...commonOpts }, { key: 'metrics', title: 'Metrics', options: props.metricsOpts, value: props.metrics, ...commonOpts }, ];

ascott · 2016-10-07T18:30:22Z

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

+  const selects = [];
+  selects.push({
+    key: 'groupByColumns',
+    title: 'GroupBy Column',


should this be Group By rather then GroupBy Column?

ascott · 2016-10-07T18:31:24Z

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

+    width: '12',
+  });
+  return (
+    <div className="panel space-1">


i don't think space-1 is a util class bootstrap has... we could add these utils to cosmo/bootswatch.less

ascott · 2016-10-07T18:32:36Z

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

+    title: 'Orderings',
+    options: props.orderingOpts,
+    value: props.orderings,
+    multi: true,


since most of the selects use the same multi and width values, could we add them as default values in the SelectArray component?

ascott · 2016-10-07T18:36:24Z

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

-        </div>
+const TimeFilter = (props) => {
+  const timeColumnTitle =
+    (props.datasourceType === 'table') ? 'Time Column' : 'Time Granularity';


could DRY this up to

const isDatasourceTypeTable = props.datasourceType === 'table' const timeColumnTitle = isDatasourceTypeTable ? 'Time Column' : 'Time Granularity'; const timeGrainTitle = isDatasourceTypeTable ? 'Time Grain' : 'Origin';

vera-liu · 2016-10-07T20:39:13Z

Emailed codeclimate's support team for the syntax error

ascott · 2016-10-07T20:49:37Z

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

@@ -2,71 +2,50 @@ import { defaultFormData, defaultOpts } from '../stores/store';
 import * as actions from '../actions/exploreActions';
 import { addToArr, removeFromArr, alterInArr } from '../../../utils/reducerUtils';

+const setFormInViz = function (state, action) {
+  const newForm = Object.assign({}, state);


should this be called newFormData for consistency?

good point, changed:)

ascott · 2016-10-07T21:24:48Z

nice PR! 🚢 💯

)

bkyryliuk reviewed Oct 7, 2016

View reviewed changes

vera-liu mentioned this pull request Oct 7, 2016

[Change in Progress] Added Advcanced Analytics panel and Chart Options for Line chart #1267

Closed

vera-liu force-pushed the vliu_restructure_store branch from f82f5ce to 59dfa38 Compare October 7, 2016 18:19

ascott reviewed Oct 7, 2016

View reviewed changes

vera-liu added 2 commits October 7, 2016 13:27

Put formData in store

cd0d335

Reform actions and reducers

be5020c

vera-liu force-pushed the vliu_restructure_store branch from 2ea9bbd to 3a51d45 Compare October 7, 2016 20:27

ascott reviewed Oct 7, 2016

View reviewed changes

Maded modifications based on comments:

184134e

vera-liu force-pushed the vliu_restructure_store branch from 3a51d45 to 184134e Compare October 7, 2016 20:56

Merge branch 'master' into vliu_restructure_store

16865f3

vera-liu merged commit b7d1f78 into apache:master Oct 7, 2016

dpgaspar mentioned this pull request Feb 19, 2020

Bump FAB to 2.2.4 #9165

Merged

12 tasks

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021

feat(plugin-chart-table): add support for temporal x-filter (apache#1281

6e714f2

)

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021

feat(plugin-chart-table): add support for temporal x-filter (apache#1281

55554ed

)

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021

feat(plugin-chart-table): add support for temporal x-filter (apache#1281

512fb7a

)

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021

feat(plugin-chart-table): add support for temporal x-filter (apache#1281

7040050

)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put formData in store #1281

Put formData in store #1281

vera-liu commented Oct 7, 2016 •

edited

bkyryliuk Oct 7, 2016

vera-liu Oct 7, 2016

bkyryliuk Oct 7, 2016

vera-liu Oct 7, 2016

ascott Oct 7, 2016

bkyryliuk Oct 7, 2016

bkyryliuk Oct 7, 2016

vera-liu Oct 7, 2016

bkyryliuk Oct 7, 2016

bkyryliuk Oct 7, 2016

vera-liu Oct 7, 2016

ascott Oct 7, 2016

vera-liu Oct 7, 2016

ascott Oct 7, 2016

vera-liu Oct 7, 2016

ascott Oct 7, 2016 •

edited

vera-liu Oct 7, 2016

ascott Oct 7, 2016

ascott Oct 7, 2016

vera-liu Oct 7, 2016

vera-liu commented Oct 7, 2016

ascott Oct 7, 2016

vera-liu Oct 7, 2016

ascott commented Oct 7, 2016

Put formData in store #1281

Put formData in store #1281

Conversation

vera-liu commented Oct 7, 2016 • edited

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

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

ascott Oct 7, 2016 • edited

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 Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascott commented Oct 7, 2016

vera-liu commented Oct 7, 2016 •

edited

ascott Oct 7, 2016 •

edited