feat(native-filters): add optional time col to time range #15117

villebro · 2021-06-11T12:56:59Z

SUMMARY

Adds an optional time column option to the filter config modal to achieve full feature parity with Filter Box.

BEFORE

Currently time ranges can only be applied to the default time column:

AFTER

When no time range is specified, the time column control is hidden:

When a time range is specified, the optional time column control is exposed (when left unpopulated, the default time column is used):

In addition, the sort metric is moved to a row of it's own, and a tooltip is added to the control:

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: closes [native filters] Select filter should allow user pick time column #14744
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

villebro · 2021-06-11T12:57:44Z

superset-frontend/src/dashboard/components/nativeFilters/utils.ts

-    granularity_sqla: 'ds',
+    granularity_sqla,


kgabryje

LGTM

kgabryje · 2021-06-11T13:52:16Z

A couple of points to discuss:

Should the time range picker and time column be in 1 row? That would strongly suggest to the user that those 2 fields are closely related. In my opinion, due to a rather large vertical distance between time range and time column, they seem like separate values at first glance
Should we prefill time column with the default value rather than leaving it empty? That would make it clearer which column is being used, especially when user doesn't know which temporal column is default

codecov · 2021-06-11T13:54:38Z

Codecov Report

Merging #15117 (1ff0498) into master (5e825cf) will decrease coverage by 0.01%.
The diff coverage is 56.00%.

❗ Current head 1ff0498 differs from pull request most recent head 8ebb589. Consider uploading reports for the commit 8ebb589 to get more accurate results

@@            Coverage Diff             @@
##           master   #15117      +/-   ##
==========================================
- Coverage   77.54%   77.52%   -0.02%     
==========================================
  Files         967      967              
  Lines       49759    49798      +39     
  Branches     6352     6364      +12     
==========================================
+ Hits        38585    38607      +22     
- Misses      10972    10989      +17     
  Partials      202      202

Flag	Coverage Δ
javascript	`72.42% <56.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/dashboard/actions/hydrate.js	`1.72% <ø> (ø)`
...mponents/nativeFilters/FiltersConfigModal/utils.ts	`66.25% <ø> (ø)`
...nd/src/dashboard/components/nativeFilters/utils.ts	`86.25% <ø> (ø)`
...nd/src/dashboard/containers/DashboardComponent.jsx	`92.30% <ø> (ø)`
...-frontend/src/dashboard/reducers/dashboardState.js	`73.33% <0.00%> (ø)`
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	`68.83% <16.66%> (-1.24%)`	⬇️
...t-frontend/src/dashboard/actions/dashboardState.js	`28.65% <33.33%> (ø)`
...nd/src/dashboard/components/nativeFilters/state.ts	`75.00% <64.10%> (-25.00%)`	⬇️
...d/src/dashboard/components/gridComponents/Tabs.jsx	`86.79% <85.71%> (-0.71%)`	⬇️
...ilters/FilterBar/FilterControls/FilterControls.tsx	`80.00% <100.00%> (+2.41%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e825cf...8ebb589. Read the comment docs.

villebro · 2021-06-11T14:12:18Z

Should the time range picker and time column be in 1 row? That would strongly suggest to the user that those 2 fields are closely related. In my opinion, due to a rather large vertical distance between time range and time column, they seem like separate values at first glance

I initially had it like that, but it seemed to look better when moved to the next row (this is a similar solution as that which we've opted for in Explore). But I don't mind having it on the same line if it causes less confusion.

Should we prefill time column with the default value rather than leaving it empty? That would make it clearer which column is being used, especially when user doesn't know which temporal column is default

That's not a bad idea. I left the option to clear so that it would be possible to fall back to the default dttm column in the dataset. But I agree that it would be more explicit to always specify the time column, especially as Filter Box does it like this.

@junlincc thoughts?

junlincc · 2021-06-11T16:09:56Z

change request: select show only when there are more than 1 time columns

villebro · 2021-06-11T18:54:01Z

change request: select show only when there are more than 1 time columns

Great idea! Thanks all for the feedback, I'll implement all changes proposed here and request a new round of reviews once done.

michael-s-molina · 2021-06-14T12:48:10Z

...ashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.test.tsx

  const props = createProps();
  const { container } = render(<ColumnSelect {...(props as any)} />, {
    useRedux: true,
  });
  expect(container.children).toHaveLength(1);
+  userEvent.type(screen.getByRole('combobox'), 'column_name');
+  await waitFor(() => {


You can join them all in one waitFor

michael-s-molina · 2021-06-14T12:48:36Z

...ashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.test.tsx

+  });
+  expect(container.children).toHaveLength(1);
+  userEvent.type(screen.getByRole('combobox'), 'column_name');
+  await waitFor(() => {


Same as previous comment

michael-s-molina · 2021-06-14T12:51:57Z

One nit: the sort type and sort metric have different widths.

villebro · 2021-06-15T10:25:24Z

Merging to unblock work on a related issue (will address the test comment in a separate PR)

feat(native-filters): add optional time col to time range

8ebb589

superset-github-bot bot added the preset-io label Jun 11, 2021

pull-request-size bot added the size/L label Jun 11, 2021

villebro commented Jun 11, 2021

View reviewed changes

kgabryje approved these changes Jun 11, 2021

View reviewed changes

michael-s-molina reviewed Jun 14, 2021

View reviewed changes

villebro merged commit 9c3c3fa into apache:master Jun 15, 2021

villebro deleted the villebro/filter-time-col branch June 15, 2021 10:25

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021

feat(native-filters): add optional time col to time range (apache#15117)

9221e45

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021

feat(native-filters): add optional time col to time range (apache#15117)

bd1f78e

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

feat(native-filters): add optional time col to time range (apache#15117)

470c264

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(native-filters): add optional time col to time range #15117

feat(native-filters): add optional time col to time range #15117

villebro commented Jun 11, 2021 •

edited

Loading

villebro Jun 11, 2021

kgabryje left a comment

kgabryje commented Jun 11, 2021 •

edited

Loading

codecov bot commented Jun 11, 2021

villebro commented Jun 11, 2021 •

edited

Loading

junlincc commented Jun 11, 2021

villebro commented Jun 11, 2021

michael-s-molina Jun 14, 2021

michael-s-molina Jun 14, 2021

michael-s-molina commented Jun 14, 2021

villebro commented Jun 15, 2021

feat(native-filters): add optional time col to time range #15117

feat(native-filters): add optional time col to time range #15117

Conversation

villebro commented Jun 11, 2021 • edited Loading

SUMMARY

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro Jun 11, 2021

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

kgabryje commented Jun 11, 2021 • edited Loading

codecov bot commented Jun 11, 2021

Codecov Report

villebro commented Jun 11, 2021 • edited Loading

junlincc commented Jun 11, 2021

villebro commented Jun 11, 2021

michael-s-molina Jun 14, 2021

Choose a reason for hiding this comment

michael-s-molina Jun 14, 2021

Choose a reason for hiding this comment

michael-s-molina commented Jun 14, 2021

villebro commented Jun 15, 2021

villebro commented Jun 11, 2021 •

edited

Loading

kgabryje commented Jun 11, 2021 •

edited

Loading

villebro commented Jun 11, 2021 •

edited

Loading