chore: Improves the native filters UI/UX - iteration 7 #15017

michael-s-molina · 2021-06-07T13:10:55Z

SUMMARY

Improves the native filters UI/UX - iteration 7

Changes the column select to use the Ant Design one
Removes horizontal scroll when the column select is too wide
Creates the first tests for the FormConfigModal component
Fixes [native-filters]don't set min width in DEFAULT VALUE #15000
Makes the fixed tabs take up the whole row in the modal
Fixes the pre-filter to consider the ad-hoc or time range when validating

This work is part of the Native dashboard filter project

The items below will be handled in the next iterations:

Unify the select components to use the AntD one. Currently, we have different selects with different themes and interactions.
Comments from @rusackas

The iterations below are optional but recommended:

Split the FiltersConfigForm into smaller components to make it easier to read and remove react warnings

@villebro @rusackas @junlincc

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

120873588-8e96b480-c557-11eb-9a8f-c60cf21f2915.mov

screen-recording-2021-06-07-at-100112-am_M1Srewlk.mov

TESTING INSTRUCTIONS

Check the before and after videos to see all the problems and fixes.

ADDITIONAL INFORMATION

Has associated issue:
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

junlincc · 2021-06-07T22:50:29Z

Thank you so much @michael-s-molina for the PR,
small comment, let let the fixed tabs take up the whole row in the modal

michael-s-molina · 2021-06-08T12:14:05Z

Thank you so much @michael-s-molina for the PR,
small comment, let let the fixed tabs take up the whole row in the modal

@junlincc has eagle eyes!

michael-s-molina · 2021-06-08T12:27:10Z

@junlincc @villebro

Latest changes added to the PR's description:

Makes the fixed tabs take up the whole row in the modal
Fixes the pre-filter to consider the ad-hoc or time range when validating

codecov · 2021-06-08T14:36:49Z

Codecov Report

Merging #15017 (6f46673) into master (0e07a5c) will increase coverage by 0.04%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##           master   #15017      +/-   ##
==========================================
+ Coverage   77.64%   77.69%   +0.04%     
==========================================
  Files         966      966              
  Lines       49615    49634      +19     
  Branches     6311     6318       +7     
==========================================
+ Hits        38524    38563      +39     
+ Misses      10890    10870      -20     
  Partials      201      201

Flag	Coverage Δ
javascript	`72.88% <84.21%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	`25.00% <0.00%> (-1.32%)`	⬇️
...d/src/filters/components/Time/TimeFilterPlugin.tsx	`100.00% <ø> (ø)`
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	`71.58% <77.77%> (+1.54%)`	⬆️
...tersConfigModal/FiltersConfigForm/ColumnSelect.tsx	`97.14% <94.73%> (-0.08%)`	⬇️
...nd/src/components/SupersetResourceSelect/index.tsx	`70.96% <0.00%> (-12.91%)`	⬇️
...mponents/nativeFilters/FiltersConfigModal/utils.ts	`66.25% <0.00%> (-1.25%)`	⬇️
...-frontend/src/explore/components/ControlHeader.jsx	`85.71% <0.00%> (ø)`
...ConfigModal/FiltersConfigForm/FilterScope/utils.ts	`98.50% <0.00%> (+0.06%)`	⬆️
...d/components/DashboardBuilder/DashboardBuilder.tsx	`91.02% <0.00%> (+0.11%)`	⬆️
... and 7 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 0e07a5c...6f46673. Read the comment docs.

junlincc · 2021-06-08T15:44:02Z

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true

github-actions · 2021-06-08T15:49:50Z

@junlincc Ephemeral environment spinning up at http://34.217.133.137:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

junlincc

LGTM, Product sign-off Thank you for the PR!!

rusackas · 2021-06-08T19:55:18Z

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx

@@ -57,8 +53,25 @@ class MainPreset extends Preset {
  }
 }

-fetchMock.get(`glob:*/api/v1/dataset/${datasourceId}`, {
-  result: [mockDatasource[fullDatasourceId]],
+fetchMock.get(`glob:*/api/v1/dataset/1`, {


Curious why we need to move away from the mockDatasource - whether it's for simplicity, or if there's something that indicates an incompatibility between the mock data and the test itself.

Right on the money... incompatibility and simplicity!

Followup question - do we need to update the mockDatasource in any way so that it's not incompatible? Just don't want to carry any tech debt forward.

I can try to update in the next iteration and see if it raises other problems.

rusackas · 2021-06-08T20:44:56Z

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

@@ -164,6 +164,8 @@ const StyledCollapse = styled(Collapse)`
 const StyledTabs = styled(Tabs)`
  .ant-tabs-nav {
    position: sticky;
+    margin-left: ${({ theme }) => theme.gridUnit * -4}px;


I think there's a better way to fix this, rather than resorting to negative margins. We (I?) can address this in a follow-up if needed, but I believe we can do the following:

Remove these margin left/right styles here.

In FilterTabsContainer:

& > .ant-tabs-content-holder -> remove the padding-right

&.ant-tabs-left > .ant-tabs-content-holder > .ant-tabs-content > .ant-tabs-tabpane -> set the padding-left to 0 (or remove the line, but then a 24px padding comes into play from somewhere, not sure if we can kill that).

add a selector for .ant-tabs-content-holder with padding: 0 ${({ theme }) => theme.gridUnit * 4}px;

Then we'll have fewer styles that are not fighting each other.

Thanks for the tip! I'll change it in the next iteration.

rusackas · 2021-06-08T20:49:18Z

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

-  font-family: SimSun, sans-serif;
-  margin-right: ${({ theme }) => theme.gridUnit - 1}px;
+  font-size: ${({ theme }) => theme.typography.sizes.s}px;
+  margin-left: ${({ theme }) => theme.gridUnit - 1}px;


nit/question: not sure by looking exactly where this applies, but based on a hunch from the code, it might be a use case for an em rather than a gridUnit, or perhaps a transform: translateX(-100%); - just to tie the positioning of the asterisk to the content itself, rather than to the gridUnit, which could change.

I agree. em will be better here. I'll change it in the next iteration.

rusackas · 2021-06-08T20:52:57Z

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

@@ -458,6 +460,28 @@ const FiltersConfigForm = (
    forceUpdate();
  };

+  const validatePreFilter = () =>
+    setTimeout(


curious: why is the setTimeout needed?

Ahhh, I see the // execute after render comment - that's fine I guess, but would a useEffect hook in the CollapsibleControl component be an option instead?

I can investigate if this is possible in the next iteration.

rusackas · 2021-06-08T21:11:30Z

...ontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx

-        ],
-        table_name: 'birth_names',
+fetchMock.get('glob:*/api/v1/dataset/1', {
+  description_columns: {},


This mock looks an awful lot like the one in Filterbar.test.tsx - should we recycle/export?

Nice catch. In the next iteration, I'll try to unify all mocks.

rusackas · 2021-06-08T21:14:34Z

...ontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx

+});
+
+// eslint-disable-next-line jest/no-disabled-tests
+test.skip('validates the default value', async () => {


Curious about the status any test that's both new AND skipped ;) Something to follow up on?

I decided to skip it and tackle it in the next iteration because it's passing locally but failing on Github for no apparent reason.

rusackas · 2021-06-08T21:16:06Z

...ontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx

+    changes the default value options when the column changes
+    switches to configuration tab when validation fails
+    displays cancel message when there are pending operations
+    do not displays cancel message when there are no pending operations


Suggested change

do not displays cancel message when there are no pending operations

does not display cancel message when there are no pending operations

obviously just a nit... really appreciate this TODO comment!

rusackas

I found a few things that might be worth consideration, but I don't think anything should be blocking really, so I'll approve. I'll leave this open for a bit, as I'd love your thoughts on the comments.

michael-s-molina · 2021-06-10T10:45:15Z

I found a few things that might be worth consideration, but I don't think anything should be blocking really, so I'll approve. I'll leave this open for a bit, as I'd love your thoughts on the comments.

@rusackas Thanks for your review. All your points added much value. I'll tackle them all in the next iteration.

The items below will be handled in the next iterations:

Unify the select components to use the AntD one. Currently, we have different selects with different themes and interactions.
Comments from @rusackas

villebro · 2021-06-10T12:12:54Z

Thank you so much @michael-s-molina for the PR,
small comment, let let the fixed tabs take up the whole row in the modal

@junlincc has eagle eyes!

I agree! I can hardly find a carton of milk in the fridge, let alone spot stuff like this 😆

github-actions · 2021-06-10T12:47:17Z

Ephemeral environment shutdown and build artifacts deleted.

superset-github-bot bot added the Turing label Jun 7, 2021

pull-request-size bot added the size/XL label Jun 7, 2021

michael-s-molina force-pushed the revamp-native-filters-7 branch 2 times, most recently from a1abe9d to fbfdadd Compare June 7, 2021 14:00

junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jun 7, 2021

michael-s-molina force-pushed the revamp-native-filters-7 branch from fbfdadd to 727911a Compare June 8, 2021 12:24

michael-s-molina force-pushed the revamp-native-filters-7 branch from 727911a to 1a32faf Compare June 8, 2021 13:48

chore: Improves the native filters UI/UX - iteration 7

6f46673

michael-s-molina force-pushed the revamp-native-filters-7 branch from 1a32faf to 6f46673 Compare June 8, 2021 14:16

junlincc requested review from junlincc and rusackas June 8, 2021 17:49

junlincc reviewed Jun 8, 2021

View reviewed changes

rusackas reviewed Jun 8, 2021

View reviewed changes

rusackas approved these changes Jun 9, 2021

View reviewed changes

villebro approved these changes Jun 10, 2021

View reviewed changes

villebro merged commit 1468026 into apache:master Jun 10, 2021

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

chore: Improves the native filters UI/UX - iteration 7 (apache#15017)

134cb17

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

chore: Improves the native filters UI/UX - iteration 7 (apache#15017)

a51e252

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

chore: Improves the native filters UI/UX - iteration 7 (apache#15017)

e3ecabb

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

chore: Improves the native filters UI/UX - iteration 7 #15017

chore: Improves the native filters UI/UX - iteration 7 #15017

michael-s-molina commented Jun 7, 2021 •

edited

Loading

junlincc commented Jun 7, 2021

michael-s-molina commented Jun 8, 2021

michael-s-molina commented Jun 8, 2021

codecov bot commented Jun 8, 2021

junlincc commented Jun 8, 2021

github-actions bot commented Jun 8, 2021

junlincc left a comment

rusackas Jun 8, 2021

michael-s-molina Jun 8, 2021

rusackas Jun 8, 2021

michael-s-molina Jun 10, 2021

rusackas Jun 8, 2021

michael-s-molina Jun 10, 2021

rusackas Jun 8, 2021

michael-s-molina Jun 10, 2021

rusackas Jun 8, 2021

rusackas Jun 8, 2021 •

edited

Loading

michael-s-molina Jun 10, 2021

rusackas Jun 8, 2021

michael-s-molina Jun 10, 2021

rusackas Jun 8, 2021

michael-s-molina Jun 10, 2021

rusackas Jun 8, 2021

rusackas left a comment

michael-s-molina commented Jun 10, 2021 •

edited

Loading

villebro commented Jun 10, 2021

github-actions bot commented Jun 10, 2021

	do not displays cancel message when there are no pending operations
	does not display cancel message when there are no pending operations

chore: Improves the native filters UI/UX - iteration 7 #15017

chore: Improves the native filters UI/UX - iteration 7 #15017

Conversation

michael-s-molina commented Jun 7, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

junlincc commented Jun 7, 2021

michael-s-molina commented Jun 8, 2021

michael-s-molina commented Jun 8, 2021

codecov bot commented Jun 8, 2021

Codecov Report

junlincc commented Jun 8, 2021

github-actions bot commented Jun 8, 2021

junlincc 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

rusackas Jun 8, 2021 • 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

rusackas left a comment

Choose a reason for hiding this comment

michael-s-molina commented Jun 10, 2021 • edited Loading

villebro commented Jun 10, 2021

github-actions bot commented Jun 10, 2021

michael-s-molina commented Jun 7, 2021 •

edited

Loading

rusackas Jun 8, 2021 •

edited

Loading

michael-s-molina commented Jun 10, 2021 •

edited

Loading