refactor: Changes the list views to use the new Select component #16393

michael-s-molina · 2021-08-22T16:13:21Z

SUMMARY

Change the list views to use the new Select component
Adjusts the design of the date range picker to match other controls
Aligns the sort select with the other controls
Makes the layout behavior consistent when resizing the screen
Adjusts the controls to have the same width
Previously we had 'Me' and 'Admin', which are the same user, now it will only show the name of the logged user always as the first option of the select.
Adjusts the tests

@junlincc @jinghua-qa @rusackas @mistercrunch

PS: Sorry for the big PR, but everything needs to be tested as a whole.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-08-25.at.3.36.08.PM.mov

Screen.Recording.2021-09-14.at.8.36.40.AM.mov

TESTING INSTRUCTIONS

Check the videos for instructions.

ADDITIONAL INFORMATION

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

codecov · 2021-08-25T20:02:34Z

Codecov Report

Merging #16393 (2a65c11) into master (adc3d24) will decrease coverage by 0.05%.
The diff coverage is 67.22%.

@@            Coverage Diff             @@
##           master   #16393      +/-   ##
==========================================
- Coverage   76.95%   76.89%   -0.06%     
==========================================
  Files        1007     1006       -1     
  Lines       54149    54145       -4     
  Branches     7369     7457      +88     
==========================================
- Hits        41669    41634      -35     
- Misses      12240    12269      +29     
- Partials      240      242       +2

Flag	Coverage Δ
javascript	`71.16% <67.22%> (-0.13%)`	⬇️

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

Impacted Files	Coverage Δ
superset-frontend/src/components/ListView/types.ts	`100.00% <ø> (ø)`
...perset-frontend/src/views/CRUD/alert/AlertList.tsx	`75.75% <ø> (ø)`
...ews/CRUD/annotationlayers/AnnotationLayersList.tsx	`77.35% <ø> (ø)`
...d/src/views/CRUD/csstemplates/CssTemplatesList.tsx	`78.26% <ø> (ø)`
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx	`69.41% <ø> (ø)`
...t-frontend/src/views/CRUD/data/query/QueryList.tsx	`69.89% <ø> (-1.08%)`	⬇️
superset-frontend/src/views/CRUD/utils.tsx	`55.81% <5.00%> (-13.04%)`	⬇️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx	`74.35% <58.33%> (-3.63%)`	⬇️
...rontend/src/components/ListView/Filters/Select.tsx	`76.92% <70.00%> (-20.64%)`	⬇️
...rontend/src/views/CRUD/dashboard/DashboardList.tsx	`72.22% <71.42%> (+0.39%)`	⬆️
... and 13 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 adc3d24...2a65c11. Read the comment docs.

michael-s-molina · 2021-08-25T22:25:48Z

/testenv up

github-actions · 2021-08-25T22:27:37Z

@michael-s-molina Ephemeral environment spinning up at http://35.163.253.132:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

jinghua-qa · 2021-08-25T23:16:56Z

I tested in the test environment, I found that there are some fields are not supported typing, is that expected?
These are the fields I found I can not type and search:

Dashboards:
STATUS/ FAVORITE

Charts
VIZ TYPE /FAVORITE

Database
EXPOSE IN SQL LAB
AQE

Dataset
TYPE

michael-s-molina · 2021-08-26T10:50:57Z

I tested in the test environment, I found that there are some fields are not supported typing, is that expected?

@jinghua-qa Thanks for testing! I added a fix to enable searching for all controls.

michael-s-molina · 2021-08-26T13:00:24Z

/testenv up

github-actions · 2021-08-26T13:06:30Z

@michael-s-molina Ephemeral environment spinning up at http://52.12.77.56:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

superset-frontend/src/components/ListView/CardSortSelect.tsx

michael-s-molina · 2021-08-27T13:58:01Z

/testenv up

github-actions · 2021-08-27T14:02:53Z

@michael-s-molina Ephemeral environment spinning up at http://34.222.251.103:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

etr2460

some initial comments

etr2460 · 2021-08-27T15:36:02Z

superset-frontend/src/components/ListView/Filters/Select.tsx

+        totalCount: 0,
+      };
+    },
+    [], // eslint-disable-line react-hooks/exhaustive-deps


shouldn't we add in the dependencies here?

etr2460 · 2021-08-27T15:37:40Z

superset-frontend/src/components/ListView/Filters/Select.tsx

-        />
-      )}
+      <Select
+        ariaLabel={typeof Header === 'string' ? Header : emptyLabel}


since Header can be a react node too, this would mean we'd have a11y text of None for a custom rendered header. seems less than ideal

You're right. I changed it to use the header or the name or a generic 'Filter'.

etr2460 · 2021-08-27T15:38:18Z

superset-frontend/src/components/ListView/Filters/index.tsx

 function UIFilters({
  filters,
  internalFilters = [],
  updateFilterValue,
 }: UIFiltersProps) {
  return (
-    <FilterWrapper>
+    <>


do we need this? i'm a bit rusty, but i thought you could return an array as a react component too

It seems that this feature is not fully supported by Typescript yet.
https://stackoverflow.com/questions/67908439/react-typescript-how-to-fix-return-type-element-is-not-a-valid-jsx-eleme?noredirect=1&lq=1

github-actions · 2021-09-14T20:29:03Z

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

ktmud · 2021-09-14T21:17:30Z

I'm getting a JS error when trying to clear a selected value.

michael-s-molina · 2021-09-15T11:30:29Z

I'm getting a JS error when trying to clear a selected value.

Thanks for spotting that @ktmud. I fixed and also rebased to get the latest changes.

michael-s-molina · 2021-09-15T14:53:18Z

/testenv up

github-actions · 2021-09-15T14:57:45Z

@michael-s-molina Ephemeral environment spinning up at http://35.85.138.206:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

ktmud

Code lgtm but are we sure this is the design we want? Currently it jus looks like there are too many "holes" in the header section. Consistency is good for most cases, but there is also context here---IMO a simple dropdown switch used for navigation has a totally different purpose than say a multi-select control in a form intended for collecting information.

This is deviating further and further away from SIP-34 design:

I'm going to stamp as I understand the need to consolidate Select implementation, but I hope we can reconsider the design here.

ktmud · 2021-09-16T00:36:27Z

superset-frontend/src/components/DatePicker/index.tsx

+
+export const RangePicker = styled(AntdRangePicker)`
+  border-radius: ${({ theme }) => theme.gridUnit}px;
+`;


Is this change intended?

Yes. The idea is to match the border style of the Select and Input components.

ktmud · 2021-09-16T16:12:55Z

superset-frontend/src/components/ListView/Filters/Select.tsx

-  PartialThemeConfig,
-} from 'src/components/Select';
+import React, { useState, useMemo } from 'react';
+import { withTheme, SupersetThemeProps, t } from '@superset-ui/core';


It seems theme variables are not used directly by this component anymore. Can we clean up withTheme?

On a separate note, I think we should prefer useTheme in all future refactoring, too---unless for wrapping legacy class-based components. I added an entry in the CSS Styled Guide.

cc. @evans @etr2460 Lmk if you have different opinions.

withTheme removed.

michael-s-molina · 2021-09-16T17:38:45Z

This is deviating further and further away from SIP-34 design:

I'm going to stamp as I understand the need to consolidate Select implementation, but I hope we can reconsider the design here.

@ktmud During our design session we came to the same conclusion. We chose to preserve the Select consistency for now, in order to migrate the component, but @mihir174 and @jess-dillard will analyze the design and propose improvements that will be implemented in the future.

ktmud · 2021-09-16T18:31:10Z

Not sure if it'd help but here's a design I implemented for another project. Using carets instead of arrows helps.

github-actions · 2021-09-22T10:44:47Z

Ephemeral environment shutdown and build artifacts deleted.

…che#16393) * chore: Change the list views to use the new Select component * Fix Cypress tests * Enables search for all controls * Adjusts controls width * Removes 'Me' and keeps the logged user on top * Fixes tests * Uses the borderless version for the filters * Fixes the tests * Reverts the Select theme to the default * Rebases and fixes js error * Fixes failing test * Removes unused withTheme

pull-request-size bot added the size/L label Aug 22, 2021

michael-s-molina force-pushed the change-list-view-selects branch from a3d4c1c to f2507f8 Compare August 25, 2021 19:26

michael-s-molina requested review from rusackas, suddjian and villebro August 25, 2021 19:38

michael-s-molina changed the title ~~chore: Change the list views to use the new Select component~~ chore: Changes the list views to use the new Select component Aug 25, 2021

michael-s-molina marked this pull request as ready for review August 25, 2021 19:42

michael-s-molina requested a review from geido August 25, 2021 19:44

pull-request-size bot added size/XL and removed size/L labels Aug 25, 2021

junlincc added the test_priority:high label Aug 26, 2021

pkdotson reviewed Aug 26, 2021

View reviewed changes

superset-frontend/src/components/ListView/CardSortSelect.tsx Outdated Show resolved Hide resolved

michael-s-molina requested review from pkdotson and etr2460 August 26, 2021 21:25

michael-s-molina changed the title ~~chore: Changes the list views to use the new Select component~~ refactor: Changes the list views to use the new Select component Aug 27, 2021

michael-s-molina requested review from ktmud and removed request for rusackas and suddjian August 27, 2021 10:21

michael-s-molina force-pushed the change-list-view-selects branch 2 times, most recently from 6d23d30 to 79e2d6e Compare August 27, 2021 11:04

etr2460 reviewed Aug 27, 2021

View reviewed changes

michael-s-molina added 10 commits September 15, 2021 07:37

chore: Change the list views to use the new Select component

16f4037

Fix Cypress tests

d03e2a5

Enables search for all controls

87c5092

Adjusts controls width

b8943c2

Removes 'Me' and keeps the logged user on top

ec3fcd1

Fixes tests

a2a9931

Uses the borderless version for the filters

f46ecd7

Fixes the tests

7f28bce

Reverts the Select theme to the default

aa25e2b

Rebases and fixes js error

8ffeaa3

michael-s-molina force-pushed the change-list-view-selects branch from db360f3 to 8ffeaa3 Compare September 15, 2021 11:28

Fixes failing test

4fa479a

ktmud mentioned this pull request Sep 16, 2021

[TODO] Add multiple databases and schemas to the test environment #16714

Closed

ktmud approved these changes Sep 16, 2021

View reviewed changes

Removes unused withTheme

2a65c11

michael-s-molina force-pushed the change-list-view-selects branch from cef9a0d to 2a65c11 Compare September 17, 2021 11:52

michael-s-molina merged commit b6d78bf into apache:master Sep 22, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Changes the list views to use the new Select component #16393

refactor: Changes the list views to use the new Select component #16393

michael-s-molina commented Aug 22, 2021 •

edited

Loading

codecov bot commented Aug 25, 2021 •

edited

Loading

michael-s-molina commented Aug 25, 2021

github-actions bot commented Aug 25, 2021

jinghua-qa commented Aug 25, 2021

michael-s-molina commented Aug 26, 2021

michael-s-molina commented Aug 26, 2021

github-actions bot commented Aug 26, 2021

michael-s-molina commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

etr2460 left a comment

etr2460 Aug 27, 2021

michael-s-molina Sep 2, 2021

etr2460 Aug 27, 2021

michael-s-molina Sep 2, 2021

etr2460 Aug 27, 2021

michael-s-molina Aug 31, 2021

github-actions bot commented Sep 14, 2021

ktmud commented Sep 14, 2021

michael-s-molina commented Sep 15, 2021

michael-s-molina commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

ktmud left a comment

ktmud Sep 16, 2021

michael-s-molina Sep 16, 2021

ktmud Sep 16, 2021

michael-s-molina Sep 16, 2021

michael-s-molina Sep 17, 2021

michael-s-molina commented Sep 16, 2021

ktmud commented Sep 16, 2021 •

edited

Loading

github-actions bot commented Sep 22, 2021

refactor: Changes the list views to use the new Select component #16393

refactor: Changes the list views to use the new Select component #16393

Conversation

michael-s-molina commented Aug 22, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Aug 25, 2021 • edited Loading

Codecov Report

michael-s-molina commented Aug 25, 2021

github-actions bot commented Aug 25, 2021

jinghua-qa commented Aug 25, 2021

michael-s-molina commented Aug 26, 2021

michael-s-molina commented Aug 26, 2021

github-actions bot commented Aug 26, 2021

michael-s-molina commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

etr2460 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

github-actions bot commented Sep 14, 2021

ktmud commented Sep 14, 2021

michael-s-molina commented Sep 15, 2021

michael-s-molina commented Sep 15, 2021

github-actions bot commented Sep 15, 2021

ktmud 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

michael-s-molina commented Sep 16, 2021

ktmud commented Sep 16, 2021 • edited Loading

github-actions bot commented Sep 22, 2021

michael-s-molina commented Aug 22, 2021 •

edited

Loading

codecov bot commented Aug 25, 2021 •

edited

Loading

ktmud commented Sep 16, 2021 •

edited

Loading