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.

Sign up for GitHub

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

Jump to bottom

feat: setup react page with submenu for datasources listview #10642

Merged
merged 9 commits into from
Aug 20, 2020
Merged

feat: setup react page with submenu for datasources listview #10642

merged 9 commits into from
Aug 20, 2020

Conversation

nytai
Copy link
Member

@nytai nytai commented Aug 19, 2020 •
edited
Loading

SUMMARY

  • Sets up infra for rendering a react view under databaseview/list/ when ENABLE_REACT_CRUD_VIEWS config flag is enabled and SIP_34_DATABASE_UI feature flag is enabled.

  • adds a submenu to the database list page.

  • moves props shared between datasets and databases subMenu into a common file.

  • renames the "welcome" app to "crud_views" and moves the welcome view under views/CRUD/

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen Shot 2020-08-19 at 11 21 03 AM

TEST PLAN

  • FAB view renders when ENABLE_REACT_CRUD_VIEWS and SIP_34_DATASOURCE_UI are both off.

  • React view (only showing SubMenu) renders when ENABLE_REACT_CRUD_VIEWS and SIP_34_DATASOURCE_UI are both on.

  • unit tests for DatasourceList

  • CI

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

@nytai nytai marked this pull request as ready for review August 19, 2020 18:43
kalimuthu123
kalimuthu123 reviewed Aug 19, 2020
View reviewed changes
Copy link

@kalimuthu123 kalimuthu123 left a comment

Choose a reason for hiding this comment

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

good processs

riahk
riahk reviewed Aug 19, 2020
View reviewed changes
Copy link
Contributor

@riahk riahk left a comment

Choose a reason for hiding this comment

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

It's probably not in scope (especially with how routing is currently setup), but we should be aiming to get all the lists within data to share a SubMenu to avoid repeating it in each list view.

superset-frontend/src/views/CRUD/dataset/DatasetList.tsx Outdated Show resolved Hide resolved
@nytai @riahk
Update superset-frontend/src/views/CRUD/dataset/DatasetList.tsx
f29f2ff 
Co-authored-by: Moriah Kreeger <moar.riah@gmail.com>
@nytai
Copy link
Member Author

nytai commented Aug 19, 2020 •
edited
Loading

@riahk I agree, this copying of the subMenu around is less than ideal. You very much identified the challenge, the way routing is set up. Ideally routing would look something like /data/datasources and /data/datasets and the SubMenu would live under the /data route portion. What's preventing that change is that we're currently feature flagging these views and have to support legacy routes. Once we fully deprecate the legacy views (and routes, potentially) we can set up permanent redirects.

...Actually, react router now supports matching multiple routes. I'll see what I can achieve.

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2020 •
edited
Loading

Codecov Report

Merging #10642 into master will increase coverage by 0.07%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10642      +/-   ##
==========================================
+ Coverage   64.30%   64.37%   +0.07%     
==========================================
  Files         778      779       +1     
  Lines       36793    36832      +39     
  Branches     3486     3488       +2     
==========================================
+ Hits        23659    23711      +52     
+ Misses      13025    13013      -12     
+ Partials      109      108       -1
Flag Coverage Δ
#cypress 55.05% <ø> (+0.50%) ⬆️
#javascript 60.72% <90.00%> (+0.07%) ⬆️
#python 59.73% <70.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
...et-frontend/src/views/CRUD/dataset/DatasetList.tsx 73.18% <ø> (ø)
superset-frontend/src/welcome/App.tsx 0.00% <0.00%> (ø)
superset/config.py 90.34% <ø> (ø)
superset/views/database/views.py 87.50% <70.00%> (-1.03%) ⬇️
...ntend/src/views/CRUD/datasource/DatasourceList.tsx 100.00% <100.00%> (ø)
...rontend/src/visualizations/FilterBox/FilterBox.jsx 49.68% <0.00%> (-13.67%) ⬇️
.../src/dashboard/components/gridComponents/Chart.jsx 83.67% <0.00%> (-2.05%) ⬇️
...t-frontend/src/dashboard/actions/dashboardState.js 58.82% <0.00%> (-1.31%) ⬇️
superset/db_engine_specs/presto.py 82.02% <0.00%> (-0.68%) ⬇️
superset/viz.py 57.27% <0.00%> (-0.28%) ⬇️
... and 14 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 a3ac506...f29f2ff. Read the comment docs.

@mistercrunch
Copy link
Member

There are really "Databases". The term "datasource" ambiguated with "dataset". Let's use "Databases". Maybe we need a short glossary in the docs.

mistercrunch
mistercrunch reviewed Aug 20, 2020
View reviewed changes
superset-frontend/src/views/CRUD/dataset/DatasetList.tsx Outdated
{
name: 'Datasources',
label: t('Datasources'),
url: '/databaseview/list/',
Copy link
Member

Choose a reason for hiding this comment

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

I vote for moving to /database/list/. The "view" suffix is a FAB artifact from what the ModelView class was class.

@nytai
Copy link
Member Author

nytai commented Aug 20, 2020

@riahk took a shot at sharing a menu component between Datasets and Databases, but quickly realized that only half the menu is shared. There's logic that conditionally adds the "Bulk Select" and "+Dataset" actions, those are specific to the Datasets view. Instead I just moved the props that are shared into a common file.

I also renamed the "Welcome" app to something more semantic and moved the Welcome component under src/views/CRUD

@nytai nytai requested a review from rusackas August 20, 2020 17:41
rusackas
rusackas approved these changes Aug 20, 2020
View reviewed changes
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.

Looks like there are a few nits questions remaining that sound like they're being addressed, but I don't see any deal-breakers, so LGTM!

@nytai nytai merged commit 7301a36 into apache:master Aug 20, 2020
@nytai nytai deleted the tai/datasource-listview-infra branch August 20, 2020 18:42
amitmiran137 pushed a commit to amitmiran137/incubator-superset that referenced this pull request Aug 21, 2020
Merge branch 'master' into dynamic-plugin-import
cf13c0a 
* master: (43 commits)
  feat: Getting fancier with Storybook (apache#10647)
  fix: dedup groupby in viz.py while preserving order (apache#10633)
  feat: bump superset-ui for certified tag (apache#10650)
  feat: setup react page with submenu for datasources listview  (apache#10642)
  feat: add certification to metrics (apache#10630)
  feat(viz-plugins): add date formatting to pivot-table (apache#10637)
  fix: controls scroll issue (apache#10644)
  feat: Allow tests files in  /src (plus Label component tests) (apache#10634)
  fix: remove duplicated params and cache_timeout from list_columns; add viz_type to list_columns (apache#10643)
  chore: splitting button stories into separate stories (apache#10631)
  refactor: remove slice level label_colors from dashboard init load (apache#10603)
  feat: card view bulk select (apache#10607)
  style: Label styling/storybook touchups (apache#10627)
  fix: removing unsupported modal sizes (apache#10625)
  feat(datasource): remove deleted columns and update column type on metadata refresh (apache#10619)
  improve documentation for country maps (apache#10621)
  chore: npm audit fix as of 2020-08-15 (apache#10613)
  feat: dataset REST API for distinct values (apache#10595)
  chore: bump react-redux to 5.1.2, whittling console noise (apache#10602)
  fixing console error about bad html attribute (apache#10604)
  ...

# Conflicts:
#	superset-frontend/src/explore/components/ExploreViewContainer.jsx
#	superset-frontend/src/views/App.tsx
#	superset/config.py
@riahk riahk mentioned this pull request Aug 25, 2020
Merged
11 tasks
Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020
@nytai
feat: setup react page with submenu for datasources listview (apache#…
2fc9c0a 
…10642)
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@nytai @auxten
feat: setup react page with submenu for datasources listview (apache#…
ff780ba 
…10642)
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistercrunch mistercrunch mistercrunch left review comments

@riahk riahk riahk left review comments

@kalimuthu123 kalimuthu123 kalimuthu123 left review comments

@rusackas rusackas rusackas approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 0.38.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nytai @codecov-commenter @mistercrunch @rusackas @riahk @kalimuthu123