[chart] new, list view (react) #8999

nytai · 2020-01-23T02:47:07Z

SUMMARY

Similar to #8845 this swaps out the chart list view rendered by FAB with one rendered client side in react, consuming the charts api at api/v1/chart/

Bulk actions are dependent on: #8979

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

codecov-io · 2020-01-23T05:15:09Z

Codecov Report

Merging #8999 into master will increase coverage by 0.12%.
The diff coverage is 74.74%.

@@            Coverage Diff             @@
##           master    #8999      +/-   ##
==========================================
+ Coverage   59.15%   59.28%   +0.12%     
==========================================
  Files         370      371       +1     
  Lines       11818    11916      +98     
  Branches     2900     2916      +16     
==========================================
+ Hits         6991     7064      +73     
- Misses       4648     4673      +25     
  Partials      179      179

Impacted Files	Coverage Δ
...t/assets/src/views/dashboardList/DashboardList.tsx	`73.14% <ø> (-0.25%)`	⬇️
superset/assets/src/welcome/App.jsx	`0% <ø> (ø)`	⬆️
superset/assets/src/views/chartList/ChartList.tsx	`74.74% <74.74%> (ø)`

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 6cb4ce0...fd9b315. Read the comment docs.

superset/assets/src/views/chartList/ChartList.less

suddjian · 2020-01-30T19:16:46Z

superset/assets/src/views/chartList/ChartList.tsx

+      return false;
+    }
+
+    return Boolean(this.state.permissions.find((p) => p === perm));


Suggested change

return Boolean(this.state.permissions.find((p) => p === perm));

return this.state.permissions.some((p) => p === perm);

suddjian · 2020-01-30T19:18:29Z

superset/assets/src/views/chartList/ChartList.tsx

+              <span
+                role='button'
+                className='action-button'
+                onClick={handleEdit}


why not a link?

I suppose we could bring in the edit modal from #9051 later and trigger a modal instead of following the url.

That was my thinking. It's a link now, but will probably fire an action in the near future

rusackas · 2020-01-30T19:28:14Z

superset/assets/src/views/chartList/ChartList.tsx

+const PAGE_SIZE = 25;
+
+interface Props {
+  addDangerToast: (msg: string) => void;


rusackas · 2020-01-30T19:28:39Z

superset/assets/src/views/chartList/ChartList.tsx

+
+interface Props {
+  addDangerToast: (msg: string) => void;
+  addSuccessToast: (msg: string) => void;


suddjian

rusackas · 2020-01-30T20:17:38Z

superset/assets/src/views/chartList/ChartList.tsx

+      },
+      (err: any) => {
+        console.error(err);
+        this.props.addDangerToast(t('There was an issue deleting the selected dashboards'));


Suggested change

this.props.addDangerToast(t('There was an issue deleting the selected dashboards'));

this.props.addDangerToast(t(`There was an issue deleting the selected ${charts.length > 1 ? 'dashboards' : 'dashboard'}`));

^ This is probably not the right way to do it with translation factored in, but it's possible to select 1 item with bulk actions, so we may want to get the pluralization right.

rusackas · 2020-01-30T20:50:58Z

superset/assets/src/views/chartList/ChartList.tsx

+
+          <ConfirmStatusChange
+            title={t('Please confirm')}
+            description={t('Are you sure you want to delete the selected charts?')}


Nit: Could address pluralization here too.

rusackas · 2020-01-30T20:53:34Z

superset/assets/src/views/chartList/ChartList.tsx

+              if (this.canDelete) {
+                bulkActions.push({
+                  key: 'delete',
+                  name: <><i className='fa fa-trash' /> Delete</>,


Should we instead pass aname and (optional) icon and handle rendering tags on the other side?

rusackas

Nits/questions inline, but in general, LGTM! 👍

robdiciuccio · 2020-01-31T16:53:47Z

A little off-topic, but I'm not seeing any guidelines or SIPs related to TypeScript usage in Superset. Have there been any discussions about Typescript vs vanilla JS? The recent React CRUD view PRs have started introducing .tsx, but previous TypeScript usage was pretty incidental. Would love to get some consensus around this before pushing full steam ahead on TypeScript.

etr2460 · 2020-01-31T17:22:10Z

Typescript was originally added to Superset as part of SIP-9 (#6120). In general, I think new code should strive to use typescript for the safety and readability that it provides, and if someone started converting old files to .ts or .tsx, I'd personally be quite happy

etr2460

a couple comments

etr2460 · 2020-01-31T17:26:44Z

superset/assets/src/views/chartList/ChartList.tsx

+        this.props.addSuccessToast(t('Deleted') + ` ${slice_name}`);
+      },
+      (err: any) => {
+        this.props.addDangerToast(t('There was an issue deleting') + `${slice_name}`);


looks like this is missing a space before slice_name. Although I don't think this translation will actually work anyway. i believe there's a way to put %s or something in the translation string so that the full sentence gets parsed for translation. You could look in the code to find it

Looks like there's docs around this: https://github.com/apache-superset/superset-ui/tree/master/packages/superset-ui-translation

Seems simple enough

etr2460 · 2020-01-31T17:26:58Z

superset/assets/src/views/chartList/ChartList.tsx

+        if (lastFetchDataConfig) {
+          this.fetchData(lastFetchDataConfig);
+        }
+        this.props.addSuccessToast(t('Deleted') + ` ${slice_name}`);


Same comment as below about fixing the translation

superset-github-bot bot added the preset-io label Jan 23, 2020

pull-request-size bot added the size/L label Jan 23, 2020

rusackas reviewed Jan 23, 2020

View reviewed changes

superset/assets/src/views/chartList/ChartList.less Outdated Show resolved Hide resolved

nytai changed the title ~~[charts] new, list view (react)~~ [WIP] [charts] new, list view (react) Jan 23, 2020

nytai force-pushed the tai/charts-list-view branch from 82fa496 to c8a5513 Compare January 27, 2020 21:20

nytai marked this pull request as ready for review January 27, 2020 21:30

nytai force-pushed the tai/charts-list-view branch 2 times, most recently from 9cd3c73 to 3e4f9fb Compare January 28, 2020 23:31

nytai changed the title ~~[WIP] [charts] new, list view (react)~~ [charts] new, list view (react) Jan 28, 2020

pull-request-size bot added size/XL and removed size/L labels Jan 28, 2020

nytai requested review from mistercrunch and dpgaspar January 28, 2020 23:53

suddjian reviewed Jan 30, 2020

View reviewed changes

rusackas reviewed Jan 30, 2020

View reviewed changes

superset/assets/src/views/chartList/ChartList.tsx

const PAGE_SIZE = 25;

interface Props {

addDangerToast: (msg: string) => void;

Copy link

Member

rusackas Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rusackas reviewed Jan 30, 2020

View reviewed changes

suddjian approved these changes Jan 30, 2020

View reviewed changes

rusackas reviewed Jan 30, 2020

View reviewed changes

rusackas approved these changes Jan 30, 2020

View reviewed changes

etr2460 reviewed Jan 31, 2020

View reviewed changes

nytai added 2 commits February 4, 2020 14:58

[charts] new, list view (react)

0260af2

DRY up template rendering

27d01b8

nytai changed the title ~~[charts] new, list view (react)~~ [chart] new, list view (react) Feb 4, 2020

nytai force-pushed the tai/charts-list-view branch from 3e4f9fb to aac36ca Compare February 4, 2020 23:06

fix i18n

c543a7b

nytai force-pushed the tai/charts-list-view branch from aac36ca to c543a7b Compare February 4, 2020 23:34

lint package.json

fd9b315

nytai requested a review from etr2460 February 5, 2020 00:44

nytai mentioned this pull request Feb 5, 2020

[dashboard, chart] fix, add filter support to match FAB's filter functionality #9086

Merged

12 tasks

craig-rueda merged commit e5f5eed into apache:master Feb 5, 2020

craig-rueda deleted the tai/charts-list-view branch February 5, 2020 22:35

nytai mentioned this pull request Mar 10, 2020

[datasets] new, listview (react) #9197

Merged

12 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chart] new, list view (react) #8999

[chart] new, list view (react) #8999

nytai commented Jan 23, 2020 •

edited

Loading

codecov-io commented Jan 23, 2020 •

edited

Loading

suddjian Jan 30, 2020

suddjian Jan 30, 2020

suddjian Jan 30, 2020 •

edited

Loading

nytai Jan 30, 2020

rusackas Jan 30, 2020

rusackas Jan 30, 2020

suddjian left a comment

rusackas Jan 30, 2020

rusackas Jan 30, 2020

rusackas Jan 30, 2020

rusackas left a comment

robdiciuccio commented Jan 31, 2020

etr2460 commented Jan 31, 2020

etr2460 left a comment

etr2460 Jan 31, 2020

nytai Feb 2, 2020

etr2460 Jan 31, 2020

	return Boolean(this.state.permissions.find((p) => p === perm));
	return this.state.permissions.some((p) => p === perm);

	this.props.addDangerToast(t('There was an issue deleting the selected dashboards'));
	this.props.addDangerToast(t(`There was an issue deleting the selected ${charts.length > 1 ? 'dashboards' : 'dashboard'}`));

[chart] new, list view (react) #8999

[chart] new, list view (react) #8999

Conversation

nytai commented Jan 23, 2020 • edited Loading

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

codecov-io commented Jan 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suddjian Jan 30, 2020 • 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

suddjian 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

rusackas left a comment

Choose a reason for hiding this comment

robdiciuccio commented Jan 31, 2020

etr2460 commented Jan 31, 2020

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

nytai commented Jan 23, 2020 •

edited

Loading

codecov-io commented Jan 23, 2020 •

edited

Loading

suddjian Jan 30, 2020 •

edited

Loading