chore(dashboard): Integrate dashboard app into the SPA bundle #14356

suddjian · 2021-04-26T17:01:52Z

SUMMARY

Move Dashboard app's individual reducer and app initialization logic to the Single Page App /views/App
Move remaining page-specific logic from the backend to the frontend (css, title)
Convert the links in the dashboard list from anchors to react-router links

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Run the changes in ephemeral env, navigate to dashboards, edit, reload, create new dashboards, go between dashboard page and other views, try url parameters such as standalone.
Add a e2e test that navigates between pages

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

codecov · 2021-04-27T22:06:17Z

Codecov Report

Merging #14356 (06cc3ae) into master (a8781c5) will increase coverage by 0.57%.
The diff coverage is 57.74%.

❗ Current head 06cc3ae differs from pull request most recent head c999cc5. Consider uploading reports for the commit c999cc5 to get more accurate results

@@            Coverage Diff             @@
##           master   #14356      +/-   ##
==========================================
+ Coverage   77.18%   77.76%   +0.57%     
==========================================
  Files         954      957       +3     
  Lines       48184    51712    +3528     
  Branches     5977     6494     +517     
==========================================
+ Hits        37193    40213    +3020     
- Misses      10794    11297     +503     
- Partials      197      202       +5

Flag	Coverage Δ
hive	`81.42% <66.66%> (+0.66%)`	⬆️
javascript	`72.20% <57.35%> (+0.09%)`	⬆️
mysql	`81.81% <66.66%> (+0.77%)`	⬆️
postgres	`81.83% <66.66%> (+0.76%)`	⬆️
presto	`81.53% <66.66%> (+0.76%)`	⬆️
python	`82.40% <66.66%> (+0.80%)`	⬆️
sqlite	`81.47% <66.66%> (+0.78%)`	⬆️

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

Impacted Files	Coverage Δ
...rontend/src/dashboard/containers/DashboardPage.tsx	`0.00% <0.00%> (ø)`
superset-frontend/src/views/App.tsx	`0.00% <0.00%> (ø)`
superset/views/base.py	`76.19% <ø> (ø)`
...rontend/src/views/CRUD/dashboard/DashboardCard.tsx	`77.27% <66.66%> (+1.08%)`	⬆️
superset/views/core.py	`79.49% <66.66%> (+3.77%)`	⬆️
superset-frontend/src/utils/urlUtils.ts	`61.53% <75.00%> (+2.44%)`	⬆️
superset-frontend/src/views/routes.tsx	`64.86% <80.00%> (+1.22%)`	⬆️
...set-frontend/src/components/ListViewCard/index.tsx	`100.00% <100.00%> (ø)`
superset-frontend/src/components/Menu/Menu.tsx	`69.47% <100.00%> (+0.99%)`	⬆️
...rontend/src/views/CRUD/dashboard/DashboardList.tsx	`75.83% <100.00%> (+0.20%)`	⬆️
... and 70 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 a8781c5...c999cc5. Read the comment docs.

superset/views/core.py

nytai · 2021-04-29T22:26:03Z

superset-frontend/src/components/Menu/Menu.tsx

@@ -159,6 +162,10 @@ export function Menu({
 }: MenuProps) {
  const [dropdownOpen, setDropdownOpen] = useState(false);

+  // would useQueryParam here but not all apps provide a router context
+  const standalone = getUrlParam('standalone', 'boolean');
+  if (standalone) return <></>;


oh this is cool, we'll support standalone=true for all pages

In the course of writing this I also learned about standalone=2 which removes the dashboard header lol

we could have standalone be a set of bit flags where each bit is a different feature to remove /s

nytai · 2021-04-29T23:04:52Z

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

@@ -169,13 +182,14 @@ function ListViewCard({
  imgPosition = 'top',
  cover,
 }: CardProps) {
+  const Link = url && linkComponent ? linkComponent : AnchorLink;


Is this just in case this is used outside of a react-router context?

Most list view links still need to be anchor links. It's just the dashboard ones that can be <Link>s for now.

suddjian · 2021-04-30T00:01:30Z

/testenv up

github-actions · 2021-04-30T00:07:04Z

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

rusackas · 2021-04-30T22:21:49Z

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

  return (
    <StyledCard
      data-test="styled-card"
      cover={
        cover || (
          <Cover>
-            <a href={url}>
+            <Link to={url!}>


Maybe we can adda. test to make sure this renders as expected with/without the url?

superset-frontend/src/dashboard/App.jsx

rusackas · 2021-04-30T22:42:45Z

superset-frontend/src/views/CRUD/dashboard/DashboardCard.tsx

@@ -139,7 +141,7 @@ function DashboardCard({
    <CardStyles
      onClick={() => {
        if (!bulkSelectEnabled) {
-          window.location.href = dashboard.url;
+          history.push(dashboard.url);


Let's do an E2E test to go into and out of a dashboard, to make sure we can get hither and yon.

Would be good to have one that hits the dashboard url directly and one that navigates to it via the list view link

rusackas

Noted a couple opportunities for tests, but in general LGTM!!

github-actions · 2021-05-04T15:51:38Z

Ephemeral environment shutdown and build artifacts deleted.

* chore(dashboard): Integrate dashboard app into the SPA bundle * fix url params * change variable name * change title correctly * custom css * lint * remove unused file * remove content assertions from dashboard tests * fix case with missing bootstrap data * fix: respect crud views flag * crud views -> spa * remove unused dashboard templates * fix: remove unused variable * fix: missed a spot with the crudViews -> spa * router link to dashboard from dashboard list page * link using the router when in card mode * lint * fix tests, add memory router * remove dashboard app files * split up the bundle a little more * use webpack preload

…#14356) * chore(dashboard): Integrate dashboard app into the SPA bundle * fix url params * change variable name * change title correctly * custom css * lint * remove unused file * remove content assertions from dashboard tests * fix case with missing bootstrap data * fix: respect crud views flag * crud views -> spa * remove unused dashboard templates * fix: remove unused variable * fix: missed a spot with the crudViews -> spa * router link to dashboard from dashboard list page * link using the router when in card mode * lint * fix tests, add memory router * remove dashboard app files * split up the bundle a little more * use webpack preload

chore(dashboard): Integrate dashboard app into the SPA bundle

8f9c8bd

superset-github-bot bot added the preset-io label Apr 26, 2021

pull-request-size bot added the size/L label Apr 26, 2021

suddjian added 4 commits April 26, 2021 13:23

fix url params

5a57c74

change variable name

169be86

change title correctly

6faf070

custom css

a50092c

suddjian force-pushed the dashboard-spa branch from 6a52ce2 to a50092c Compare April 26, 2021 23:38

suddjian added 3 commits April 26, 2021 16:46

lint

af37380

remove unused file

30fdccd

remove content assertions from dashboard tests

505c7d8

fix case with missing bootstrap data

cfcc0af

suddjian marked this pull request as ready for review April 28, 2021 19:47

apache deleted a comment from github-actions bot Apr 28, 2021

nytai reviewed Apr 28, 2021

View reviewed changes

superset/views/core.py Outdated Show resolved Hide resolved

fix: respect crud views flag

249bf6c

suddjian force-pushed the dashboard-spa branch from 8220fb9 to 249bf6c Compare April 29, 2021 00:56

suddjian added 8 commits April 28, 2021 18:50

crud views -> spa

364f64c

remove unused dashboard templates

4bdb2c1

fix: remove unused variable

9bbc1b3

fix: missed a spot with the crudViews -> spa

d147624

router link to dashboard from dashboard list page

123a1d8

link using the router when in card mode

2fc236a

lint

fd82c87

fix tests, add memory router

c9dbdfc

suddjian requested review from nytai, etr2460 and rusackas April 29, 2021 18:17

suddjian changed the title ~~chore(dashboard): [WiP] Integrate dashboard app into the SPA bundle~~ chore(dashboard): Integrate dashboard app into the SPA bundle Apr 29, 2021

nytai reviewed Apr 29, 2021

View reviewed changes

apache deleted a comment from github-actions bot Apr 29, 2021

nytai reviewed Apr 29, 2021

View reviewed changes

remove dashboard app files

dc60122

pull-request-size bot added size/XL and removed size/L labels Apr 29, 2021

apache deleted a comment from github-actions bot Apr 30, 2021

split up the bundle a little more

06cc3ae

nytai approved these changes Apr 30, 2021

View reviewed changes

rusackas reviewed Apr 30, 2021

View reviewed changes

superset-frontend/src/dashboard/App.jsx Show resolved Hide resolved

rusackas reviewed Apr 30, 2021

View reviewed changes

rusackas approved these changes Apr 30, 2021

View reviewed changes

use webpack preload

c999cc5

suddjian merged commit 21cf12a into master May 4, 2021

suddjian deleted the dashboard-spa branch May 10, 2021 21:10

graceguo-supercat mentioned this pull request May 12, 2021

fix: bring back dashboard perf logger #14584

Merged

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(dashboard): Integrate dashboard app into the SPA bundle #14356

chore(dashboard): Integrate dashboard app into the SPA bundle #14356

suddjian commented Apr 26, 2021 •

edited

codecov bot commented Apr 27, 2021 •

edited

nytai Apr 29, 2021

suddjian Apr 29, 2021

suddjian Apr 29, 2021 •

edited

nytai Apr 29, 2021

nytai Apr 29, 2021

suddjian Apr 29, 2021

suddjian commented Apr 30, 2021

github-actions bot commented Apr 30, 2021

rusackas Apr 30, 2021

rusackas Apr 30, 2021

nytai Apr 30, 2021

rusackas left a comment

github-actions bot commented May 4, 2021

chore(dashboard): Integrate dashboard app into the SPA bundle #14356

chore(dashboard): Integrate dashboard app into the SPA bundle #14356

Conversation

suddjian commented Apr 26, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov bot commented Apr 27, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suddjian Apr 29, 2021 • edited

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 commented Apr 30, 2021

github-actions bot commented Apr 30, 2021

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

github-actions bot commented May 4, 2021

suddjian commented Apr 26, 2021 •

edited

codecov bot commented Apr 27, 2021 •

edited

suddjian Apr 29, 2021 •

edited