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: SIP-34 table list view for databases #10705

Merged
merged 2 commits into from
Sep 2, 2020
Merged

feat: SIP-34 table list view for databases #10705

merged 2 commits into from
Sep 2, 2020

Conversation

nytai
Copy link
Member

@nytai nytai commented Aug 28, 2020

SUMMARY

  • adds a table view for datasets

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:
Screen Shot 2020-08-27 at 10 23 41 PM
Screen Shot 2020-08-27 at 10 23 51 PM
Screen Shot 2020-08-27 at 10 24 08 PM
Screen Shot 2020-08-27 at 10 24 16 PM
Screen Shot 2020-08-27 at 10 24 30 PM

TEST PLAN

-CI (enzyme tests), 👀

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI (behind ENABLE_REACT_CRUD_VIEWS config flag and SIP_34_DATABASE_UI feature flag)
  • 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 28, 2020 18:40
@nytai nytai changed the title feat: list view table for databases feat: SIP-34 table list view for databases Aug 28, 2020
@nytai
Copy link
Member Author

nytai commented Aug 28, 2020

related to #8976

lilykuang
lilykuang reviewed Aug 31, 2020
View reviewed changes
superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx
@@ -96,6 +270,18 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) {
/* TODO: add database logic here */
}}
/>

<ListView<DatabaseObject>
Copy link
Member

Choose a reason for hiding this comment

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

question: just wondering what does <DatabaseObject> do here

Copy link
Member Author

Choose a reason for hiding this comment

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

It allows specifying the generic for the ListView props. This means that props like renderCard are not (data: any) => ReactNode and are instead (data: DatabaseObject) => ReactNode.

You can read more about it here: https://mariusschulz.com/articles/passing-generics-to-jsx-elements-in-typescript

riahk
riahk reviewed Aug 31, 2020
View reviewed changes
superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx Outdated Show resolved Hide resolved
superset-frontend/src/views/CRUD/data/database/types.ts
last_name: string;
};

export type DatabaseObject = {
Copy link
Contributor

@riahk riahk Aug 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure how we want to handle the DatabaseObject between the list and the modal; especially since the modal has some extra fields, do we want to include those as optional props or just create a second type to use in the modal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we expect them to be significantly different? If so we can create separate types, or create a base type and extend it for each of the instances. Ideally they wouldn't be too different.

@nytai nytai force-pushed the tai/database-list branch 4 times, most recently from 7a80c53 to dcc389f Compare September 2, 2020 17:08
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Sep 2, 2020
@pull-request-size pull-request-size bot added size/L and removed size/XL labels Sep 2, 2020
@nytai nytai force-pushed the tai/database-list branch 2 times, most recently from 94fde9a to 8e1788f Compare September 2, 2020 17:17
willbarrett
willbarrett approved these changes Sep 2, 2020
View reviewed changes
Copy link
Member

@willbarrett willbarrett left a comment

Choose a reason for hiding this comment

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

LGTM with one nit

superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx Outdated
original: { allow_dml: allowDML },
},
}: any) =>
allowDML ? <IconBlack name="check" /> : <IconBlack name="cancel-x" />,
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 ternary is repeated a number of times with the same display element. Is there an easy way to DRY it up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. Fixed!

@nytai nytai merged commit 7bccb38 into apache:master Sep 2, 2020
@nytai nytai deleted the tai/database-list branch September 2, 2020 18:48
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 7, 2020
Merge remote-tracking branch 'upstream/master' into feat/add_all_dash…
d709758 
…boards_permissions

* upstream/master: (32 commits)
  docs: Add a note to contributing.md on reporting security vulnerabilities (apache#10796)
  Fix: Include RLS filters for cache keys (apache#10805)
  feat: filters for database list view (apache#10772)
  fix: MVC show saved query (apache#10781)
  added creator column and adjusted order columns (apache#10789)
  security: disallow uuid package on jinja2 (apache#10794)
  feat: CRUD REST API for saved queries (apache#10777)
  fix: disable domain sharding on explore view (apache#10787)
  fix: can not type `0.05` in `TextControl` (apache#10778)
  fix: pivot table timestamp grouping (apache#10774)
  fix: add validator information to email/slack alerts (apache#10762)
  More Label touchups (margins) (apache#10722)
  fix: dashboard extra filters (apache#10692)
  fix: re-installing local superset in cache image (apache#10766)
  feat: SIP-34 table list view for databases (apache#10705)
  refactor: convert DatasetList schema filter to use new distinct api (apache#10746)
  chore: removing fsevents dependency (apache#10751)
  Fix precommit hook for docs/installation.rst (apache#10759)
  feat(database): POST, PUT, DELETE API endpoints (apache#10741)
  docs: Update OAuth configuration in installation.rst (apache#10748)
  ...
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@nytai @auxten
feat: SIP-34 table list view for databases (apache#10705)
0924f3b
@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

@lilykuang lilykuang lilykuang left review comments

@riahk riahk riahk approved these changes

@willbarrett willbarrett willbarrett approved these changes

@dpgaspar dpgaspar Awaiting requested review from dpgaspar

@rusackas rusackas Awaiting requested review from rusackas

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.
5 participants
@nytai @willbarrett @lilykuang @riahk @mistercrunch