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

[datasets] new, listview (react) #9197

Merged
merged 7 commits into from Mar 13, 2020

Conversation

nytai
Copy link
Member

@nytai nytai commented Feb 25, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Adds a new react listview to tables (soon to be renamed to datasets).

similar to #8845 and #8999

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen Shot 2020-03-10 at 2 08 25 PM

Screen Shot 2020-03-10 at 2 08 49 PM

TEST PLAN

  • New listview (behind config ENABLE_REACT_CRUD_VIEWS) provides the same functionality as previous MVC crud views
  • New listview is behind flag ENABLE_REACT_CRUD_VIEWS

ADDITIONAL INFORMATION

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

REVIEWERS

@nytai nytai changed the title [datasets] new, listview (react) WIP: [datasets] new, listview (react) Feb 25, 2020
@nytai nytai marked this pull request as ready for review March 10, 2020 00:37
@codecov-io
Copy link

codecov-io commented Mar 10, 2020

Codecov Report

Merging #9197 into master will increase coverage by 0.19%.
The diff coverage is 61.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9197      +/-   ##
==========================================
+ Coverage   58.90%   59.10%   +0.19%     
==========================================
  Files         373      374       +1     
  Lines       12026    12159     +133     
  Branches     2953     2982      +29     
==========================================
+ Hits         7084     7186     +102     
- Misses       4763     4794      +31     
  Partials      179      179              
Impacted Files Coverage Δ
...rset-frontend/src/SqlLab/components/QueryTable.jsx 59.25% <ø> (ø)
...uperset-frontend/src/SqlLab/components/ShowSQL.jsx 55.55% <ø> (ø)
...et-frontend/src/SqlLab/components/TableElement.jsx 82.75% <ø> (ø)
superset-frontend/src/components/Link.tsx 93.75% <ø> (ø)
...uperset-frontend/src/views/chartList/ChartList.tsx 69.09% <0.00%> (+6.36%) ⬆️
...frontend/src/views/dashboardList/DashboardList.tsx 65.85% <0.00%> (+6.50%) ⬆️
superset-frontend/src/welcome/App.jsx 0.00% <ø> (ø)
...set-frontend/src/views/datasetList/DatasetList.tsx 59.48% <59.48%> (ø)
...rset-frontend/src/components/ListView/ListView.tsx 86.86% <100.00%> (+0.84%) ⬆️
...ontend/src/components/ListView/TableCollection.tsx 90.32% <100.00%> (+0.32%) ⬆️
... and 8 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 d8fea64...22c68ea. Read the comment docs.

@nytai nytai changed the title WIP: [datasets] new, listview (react) [datasets] new, listview (react) Mar 10, 2020
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Two type nits

superset/connectors/sqla/models.py Outdated Show resolved Hide resolved
superset/connectors/sqla/models.py Outdated Show resolved Hide resolved
@nytai nytai requested a review from villebro March 10, 2020 23:54
@villebro
Copy link
Member

When playing around with this, I get the following error with the following filters:

  • SQL Lab view (checked)
  • Database (empty)

image

@nytai
Copy link
Member Author

nytai commented Mar 11, 2020

@villebro Thanks for catching that, I also found that my implementation of the database filter was incorrect. Both issues have been addressed. I've also added an exception which should help catch incorrect filter config moving forward.


handleDashboardDelete = ({ id, table_name: tableName }: Dataset) =>
SupersetClient.delete({
endpoint: `/api/v1/dashboard/${id}`,
Copy link
Member

Choose a reason for hiding this comment

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

I think you want a different endpoint here

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

There seems to be some JS lint, otherwise LGTM. This is a really nice improvement to the user experience, great work! 👍

@nytai nytai changed the title [datasets] new, listview (react) WIP: [datasets] new, listview (react) Mar 12, 2020
@nytai
Copy link
Member Author

nytai commented Mar 12, 2020

I just realized this change will remove the create button for datasets(tables). For Charts and Dashboards there was the NEW button to fall back on, however that button is missing Tables. This is a blocker, I'll have to figure something out

@nytai
Copy link
Member Author

nytai commented Mar 13, 2020

Added a + button

Screen Shot 2020-03-12 at 6 35 10 PM

@nytai nytai changed the title WIP: [datasets] new, listview (react) [datasets] new, listview (react) Mar 13, 2020
@villebro villebro merged commit 5767fb1 into apache:master Mar 13, 2020
@mistercrunch 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
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/XL 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants