[dashboard] New, list view (react) #8845

nytai · 2019-12-16T19:22:19Z

SUMMARY

Replaces the current dashboard list view generated by FAB with one that is client rendered in react using the FAB/dashboard api. Design has tried to stay as close to the current design as possible, with some minor improvements. There have been recent efforts to redesign most of the views in superset, so having pages that are rendered in react will greatly facilitate those changes. The work in this PR is intended to lay the foundation for all list views that are to come.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

old view

new view

TEST PLAN

The welcome page works as before.
The dashboards page renders, sorts, and filters

ADDITIONAL INFORMATION

REVIEWERS

superset/assets/src/dashboardList/DashboardList.tsx

aboganas · 2019-12-18T14:25:42Z

This is awesome, I hope this be ported to Flask-AppBuilder lists

superset/models/dashboard.py

nytai · 2019-12-20T17:21:55Z

superset/assets/jest.config.js

+  globals: {
+    'ts-jest': {
+      diagnostics: {
+        warnOnly: true,


so type errors don't cause jest tests to fail (they're still reported though)

nytai · 2019-12-20T17:24:07Z

superset/assets/src/types/react-table.d.ts

@@ -0,0 +1,224 @@
+// Type definitions for react-table 7


the newest version of react-table doesn't have types in https://github.com/DefinitelyTyped/DefinitelyTyped

found these online and adjusted to for the features we're using.

nytai · 2019-12-20T17:27:42Z

superset/assets/tslint.json

+    "quotemark": [true, "single"],
+    "jsx-no-multiline-js": false,
+    "jsx-no-lambda": false


We don't have these rules enabled in eslint and I found them to be very tedious. Opened #8865 for larger discussion

nytai · 2019-12-20T17:29:29Z

superset/views/dashboard/views.py

@@ -102,6 +105,21 @@ def new(self):  # pylint: disable=no-self-use
        db.session.commit()
        return redirect(f"/superset/dashboard/{new_dashboard.id}/?edit=true")

+    @has_access


found I had to run superset init after adding this decorator. Not sure if there's a way around it. This is necessary to override FAB's default list view.

One possible way could be to override with:

Class Dashboard(BaseSupersetView): class_permission_name = "DashboardModelView" method_permission_name = { "new": "add", "list_all": "list", }

Note: This would deprecate the Dashboard.new permission

codecov-io · 2019-12-23T00:59:56Z

Codecov Report

Merging #8845 into master will increase coverage by 0.47%.
The diff coverage is 80.56%.

@@            Coverage Diff             @@
##           master    #8845      +/-   ##
==========================================
+ Coverage   58.69%   59.16%   +0.47%     
==========================================
  Files         363      367       +4     
  Lines       11417    11679     +262     
  Branches     2801     2861      +60     
==========================================
+ Hits         6701     6910     +209     
- Misses       4538     4590      +52     
- Partials      178      179       +1

Impacted Files	Coverage Δ
superset/assets/src/welcome/App.jsx	`0% <ø> (ø)`	⬆️
...t/assets/src/views/dashboardList/DashboardList.tsx	`58.94% <58.94%> (ø)`
superset/assets/src/welcome/Welcome.jsx	`78.57% <75%> (-1.43%)`	⬇️
superset/assets/src/welcome/DashboardTable.jsx	`75% <85.18%> (-2.78%)`	⬇️
...perset/assets/src/components/ListView/ListView.tsx	`91.17% <91.17%> (ø)`
...assets/src/components/ListView/TableCollection.tsx	`92.59% <92.59%> (ø)`
superset/assets/src/components/ListView/utils.ts	`98.14% <98.14%> (ø)`
... 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 ff9bc74...8262355. Read the comment docs.

nytai · 2020-01-09T23:43:02Z

@etr2460 @graceguo-supercat @michellethomas Would be great to get some 👀 on this from folks who spend more of their time in the frontend.

dpgaspar

Tested it locally and looks good, but noticed the following:

when switching languages the user is redirected to the old dashboard list (previous MVC page)
column labels are not translated
Button labels are not translated
Edit form back button does not work work properly (previous MVC), I would say that it's only usable when using modal forms form add and edit

superset/models/dashboard.py

superset/views/dashboard/api.py

nytai · 2020-01-13T21:31:23Z

@dpgaspar I've updated with the changes your requested as best I could.

when switching languages the user is redirected to the old dashboard list (previous MVC page)

looks like the user is redirected to the edit page of a specific dashboard, not sure how that's happening but doubt it's due to any change I made. Seems like that url is exposed in FAB and I don't really understand how that works right now.

column labels are not translated

I've updated the code to use the label_columns data provided by the backend. Looks like we're missing some translations there.

Button labels are not translated

I've updated the code to use the t('....') function from superset-ui/translation which seems to be the way of handling translations on the frontend. We're also likely missing most translations there.

Edit form back button does not work work properly (previous MVC), I would say that it's only usable when using modal forms form add and edit

This is true. looks like that links to /back which appears to be an endpoint in FAB? Somehow it's redirecting back to the charts list. Not sure how that works or how it's configured. -- There's work being done to move that functionality to a modal so that could be the workaround solution for that issue.

mistercrunch · 2020-01-14T19:39:11Z

superset/assets/src/views/dashboardList/DashboardList.tsx

+
+const PAGE_SIZE = 25;
+
+interface Props {


I'm unclear about the pros/cons of using interface over propTypes, assuming this is more typescript-friendly (?). Tradeoff seems to be consistency vs being more ts friendly (?)

We could use both. PropTypes are checked at runtime by react in development and log to the console. Interface/types are checked by the typescript compiler/vscode and produce errors at build time and are highlighted in the editor. I'd say typescript is favorable once the entire repo converts to typescript as the types will be available in the editor (on hover, cmd click, etc). For now I'm happy to also add PropTypes until we transition more to typescript.

FWIW: The react dev team recommends flow/typescript over PropTypes for larger projects: https://reactjs.org/docs/static-type-checking.html

mistercrunch · 2020-01-14T19:40:24Z

superset/assets/src/views/dashboardList/DashboardList.tsx

+        <Modal show={this.state.showDeleteModal} onHide={this.toggleModal}>
+          <Modal.Header closeButton={true} />
+          <Modal.Body>
+            Are you sure you want to delete{' '}


mistercrunch · 2020-01-14T19:44:09Z

superset/assets/src/welcome/DashboardTable.jsx

+    search: PropTypes.string,
+  };
+
+  state = {


Is this the same as setting state in the constructor?

Yes, they are essentially the same thing. The class properties is just sugar for setting variables in constructor. I personally prefer this over using the constructor as the constructor requires a call to super() which can sometimes be overlooked.

mistercrunch · 2020-01-14T19:53:02Z

superset/assets/src/views/dashboardList/DashboardList.tsx

+    return this.hasPerm('can_delete');
+  }
+
+  public static propTypes = {


Wondered why we haven't used this syntax before and found this:

I've added the babel plugin for transform-class-properties. The proposal for these is currently in stage 3, I believe, which is the step right before formal adoption into the standard. The proposal states that implementation is already under way in the latest chrome and in node 12.
proposal:
https://github.com/tc39/proposal-class-fields#class-field-declarations-for-javascript

Happy to switch back too. FWIW, every react project I've worked on has had this syntax enabled by babel.

superset-github-bot bot added the preset-io label Dec 16, 2019

pull-request-size bot added the size/XXL label Dec 16, 2019

nytai force-pushed the tai/list-view branch 2 times, most recently from 131466e to 319e011 Compare December 16, 2019 21:20

dpgaspar reviewed Dec 17, 2019

View reviewed changes

superset/assets/src/dashboardList/DashboardList.tsx Outdated Show resolved Hide resolved

nytai changed the title ~~new, Dashboard list view (react)~~ [WIP] new, Dashboard list view (react) Dec 18, 2019

nytai marked this pull request as ready for review December 18, 2019 00:20

nytai force-pushed the tai/list-view branch from a4c78f9 to bf13c63 Compare December 18, 2019 00:24

dpgaspar reviewed Dec 18, 2019

View reviewed changes

superset/assets/src/dashboardList/DashboardList.tsx Outdated Show resolved Hide resolved

nytai force-pushed the tai/list-view branch 2 times, most recently from 6f00ff4 to ebeebe3 Compare December 18, 2019 22:57

dpgaspar reviewed Dec 19, 2019

View reviewed changes

superset/models/dashboard.py Outdated Show resolved Hide resolved

nytai force-pushed the tai/list-view branch from 6c46454 to de453f7 Compare December 20, 2019 16:44

nytai commented Dec 20, 2019

View reviewed changes

nytai changed the title ~~[WIP] new, Dashboard list view (react)~~ new, Dashboard list view (react) Dec 23, 2019

dpgaspar changed the title ~~new, Dashboard list view (react)~~ [dashboard] New, list view (react) Jan 3, 2020

nytai force-pushed the tai/list-view branch from b174647 to 78e5b1f Compare January 4, 2020 06:13

dpgaspar requested changes Jan 10, 2020

View reviewed changes

superset/models/dashboard.py Outdated Show resolved Hide resolved

superset/views/dashboard/api.py Outdated Show resolved Hide resolved

nytai requested a review from dpgaspar January 13, 2020 21:31

mistercrunch reviewed Jan 14, 2020

View reviewed changes

nytai added 21 commits January 15, 2020 15:54

fix double import from bad rebase

f89e267

fix indent error

754b489

lookup permissions

eb4f50e

generic permission lookup

cd9a0fa

get tslint to pass

bb8f3c9

adds js specs

dae5e3f

lint

64857a3

fix rebase

a36d3c1

lint

b3a14ba

lint again

d8f3943

fix type errors preventing build

99d321a

adds more specs

5dc799e

fix tslint error

fb043e4

fix null check

95a7016

remove unecessary code

a6a040b

use translations provided by api

e6de4c8

more translations

5a50fcc

linting

00551c5

fix spec

2485ca0

i18n

d026df4

fix register order

8262355

nytai force-pushed the tai/list-view branch from a6b7991 to 8262355 Compare January 15, 2020 23:56

nytai requested a review from mistercrunch January 15, 2020 23:58

mistercrunch approved these changes Jan 16, 2020

View reviewed changes

mistercrunch merged commit 7b97764 into apache:master Jan 16, 2020

This was referenced Jan 23, 2020

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

Merged

[dashboard] fix, enable info endpoint #9015

Merged

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

[dashboard] New, list view (react) #8845

[dashboard] New, list view (react) #8845

nytai commented Dec 16, 2019 •

edited

Loading

aboganas commented Dec 18, 2019

nytai Dec 20, 2019

nytai Dec 20, 2019

nytai Dec 20, 2019

nytai Dec 20, 2019

dpgaspar Jan 3, 2020 •

edited

Loading

codecov-io commented Dec 23, 2019 •

edited

Loading

nytai commented Jan 9, 2020

dpgaspar left a comment

nytai commented Jan 13, 2020

mistercrunch Jan 14, 2020

nytai Jan 15, 2020

mistercrunch Jan 14, 2020

mistercrunch Jan 14, 2020

nytai Jan 15, 2020

mistercrunch Jan 14, 2020

nytai Jan 15, 2020 •

edited

Loading

[dashboard] New, list view (react) #8845

[dashboard] New, list view (react) #8845

Conversation

nytai commented Dec 16, 2019 • edited Loading

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

old view

new view

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

aboganas commented Dec 18, 2019

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

dpgaspar Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Dec 23, 2019 • edited Loading

Codecov Report

nytai commented Jan 9, 2020

dpgaspar left a comment

Choose a reason for hiding this comment

nytai commented Jan 13, 2020

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

nytai Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

nytai commented Dec 16, 2019 •

edited

Loading

dpgaspar Jan 3, 2020 •

edited

Loading

codecov-io commented Dec 23, 2019 •

edited

Loading

nytai Jan 15, 2020 •

edited

Loading