Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: home screen mvp #11206

Merged
merged 46 commits into from
Oct 30, 2020
Merged

feat: home screen mvp #11206

merged 46 commits into from
Oct 30, 2020

Conversation

pkdotson
Copy link
Member

@pkdotson pkdotson commented Oct 8, 2020

SUMMARY

This PR introduces the new homescreen that the user will first interact with on login. The pr also decouples the card components from chartlist and dashboardlist components so that they both can be used more readily throughout homescreen.

Screen Shot 2020-10-19 at 11 06 14 AM

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Test Plan includes unit test and end to end test which are in development and will be updated during this review.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • [x ] Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #11206 into master will decrease coverage by 0.04%.
The diff coverage is 62.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11206      +/-   ##
==========================================
- Coverage   66.61%   66.56%   -0.05%     
==========================================
  Files         862      869       +7     
  Lines       41215    41587     +372     
  Branches     3719     3797      +78     
==========================================
+ Hits        27454    27684     +230     
- Misses      13660    13801     +141     
- Partials      101      102       +1     
Flag Coverage Δ
#cypress 55.81% <20.72%> (-0.62%) ⬇️
#javascript 62.57% <59.71%> (-0.13%) ⬇️
#python 62.03% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/views/CRUD/data/common.ts 100.00% <ø> (ø)
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx 61.83% <ø> (ø)
superset/charts/api.py 76.98% <ø> (ø)
superset/dashboards/api.py 82.87% <ø> (ø)
superset/queries/api.py 100.00% <ø> (ø)
superset-frontend/src/views/CRUD/hooks.ts 63.77% <27.02%> (-15.11%) ⬇️
superset-frontend/src/views/CRUD/utils.tsx 50.00% <30.50%> (-50.00%) ⬇️
...-frontend/src/views/CRUD/welcome/ActivityTable.tsx 46.47% <46.47%> (ø)
...perset-frontend/src/views/CRUD/welcome/Welcome.tsx 60.00% <52.94%> (-16.00%) ⬇️
...frontend/src/views/CRUD/welcome/DashboardTable.tsx 60.37% <58.82%> (-18.58%) ⬇️
... and 27 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 ed3dca4...3af819d. Read the comment docs.

@pkdotson pkdotson marked this pull request as ready for review October 19, 2020 21:21
@junlincc junlincc requested review from mistercrunch and removed request for suddjian October 21, 2020 23:47
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Lots of little things noted... some more important than others, obviously. Thanks for putting this feature together!

@rusackas
Copy link
Member

A few other little things that we discussed in person. They should not be blockers here, and could be followed up on other PRs, but I wanted to put them "on paper":

  • It would be great to share/reuse the media queries with cards (keeping it DRY), so they benefit other card views.
  • Maybe instead of living in superset-frontend/src/views/CRUD/welcome it should live in superset-frontend/src/views/welcome as its own thing.
  • If _may be of benefit to decouple all the card variants (Recent/Chart/Dashboard/SavedQuery/(eventually...)SearchResult). If doing so, then they should all probably continue to use the common ListVIewCard under the hood, which could be renamed as BaseCard or similar. All of these could live in the main components directory, perhaps hierarchically nested.

@hughhhh hughhhh self-requested a review October 22, 2020 16:27
actions: React.ReactNode | null;
showImg?: boolean;
rows?: number | string;
avatar?: string;
Copy link
Member

Choose a reason for hiding this comment

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

nit: this should be IconName or IconProps["name"].

superset-frontend/src/views/CRUD/hooks.ts Show resolved Hide resolved
superset-frontend/src/views/CRUD/hooks.ts Show resolved Hide resolved
superset-frontend/src/components/ListViewCard/index.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/ListViewCard/index.tsx Outdated Show resolved Hide resolved
superset-frontend/src/views/CRUD/utils.tsx Show resolved Hide resolved
superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx Outdated Show resolved Hide resolved
superset-frontend/src/views/CRUD/welcome/EmptyState.tsx Outdated Show resolved Hide resolved
superset-frontend/src/views/CRUD/chart/ChartCard.tsx Outdated Show resolved Hide resolved
@nytai
Copy link
Member

nytai commented Oct 27, 2020

FYI, @riahk also created an empty list state in #11432

@rusackas
Copy link
Member

Couldn't continue with reviewing because this PR really needs a rebase. The diff is too large.

After rebasing, you should have only 35 changed files instead of 119.
master...ktmud:home-screen-mvp

All cleared up now, back to 34 files. Thanks for your input so far. Good to have fresh eyes on this.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

OK... I think this thing is ready to go. Seems like all the serious concerns have been addressed. Plenty more great suggestions from those who have taken the time to review this (thank you!). I think any remaining suggestions/advice can be followed up on in subsequent PRs.

@rusackas rusackas merged commit f7051ea into apache:master Oct 30, 2020
@ktmud
Copy link
Member

ktmud commented Oct 30, 2020

A huge milestone! Thanks for all the great work @pkdotson and bearing with the back and forth. Looking forward to more exciting improvements to come!

@junlincc
Copy link
Member

🥺 thank you @pkdotson so proud of you!!

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* step 1: broken stuff!

* first steps

* more adding and slicing

* step 1: broken stuff!

* can now filter dashboards/charts for "Edited" tabs (filter by changed_by o_m)

* more updates

* update recent card

* add icon

* Adding Expand Icon to Collapse component

* more updates

* clean up code

* remove lock file

* remove consoles

* fixing subnav button height shift

* lil' ascii arrows

* update branch

* update test part 1

* remove consoles

* fix typescript

* add images and update emptystate

* add changes

* update chart card

* fix css issues from rebase

* add suggestions

* more changes

* update tests and clear typescript errors

* Update superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

Co-authored-by: Evan Rusackas <evan@preset.io>

* update from comments

* more updates..

* fix rebase

* fix pesky type errors

* test fixes

* lint fix

* Update superset-frontend/spec/javascripts/views/CRUD/welcome/Welcome_spec.tsx

Co-authored-by: Evan Rusackas <evan@preset.io>

* Update superset-frontend/src/views/CRUD/welcome/EmptyState.tsx

Co-authored-by: Evan Rusackas <evan@preset.io>

* Update superset-frontend/src/components/Menu/SubMenu.tsx

Co-authored-by: Evan Rusackas <evan@preset.io>

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

Co-authored-by: ʈᵃᵢ <tdupreetan@gmail.com>

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

Co-authored-by: ʈᵃᵢ <tdupreetan@gmail.com>

* add suggestions

* fix lint

* remove unused code

* toast getrecentActivityobjs

* add some suggestions

* remove types for now

* cypress fix

* remove unused type

Co-authored-by: Evan Rusackas <evan@preset.io>
Co-authored-by: ʈᵃᵢ <tdupreetan@gmail.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants