-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(listviews): SIP-34 filters for charts, dashboards, datasets #10335
Conversation
fe39e3f
to
86284b4
Compare
437e504
to
b44cf7e
Compare
Codecov Report
@@ Coverage Diff @@
## master #10335 +/- ##
==========================================
- Coverage 70.25% 70.16% -0.10%
==========================================
Files 605 605
Lines 32425 32386 -39
Branches 3295 3270 -25
==========================================
- Hits 22781 22724 -57
- Misses 9535 9553 +18
Partials 109 109
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
can't wait to see it live! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first pass, couple of comments
superset/databases/api.py
Outdated
page: | ||
type: integer | ||
filter: | ||
type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nicer to use a reference here to the schema, so we don't have to repeat ourselfs. Take a look at: https://github.com/apache/incubator-superset/blob/master/superset/charts/api.py#L502
superset/databases/api.py
Outdated
value: | ||
type: string | ||
text: | ||
type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also, declare a marshmallow schema for the response and reference that. Register your schema on OpenAPI using openapi_spec_component_schemas
FAB attr
84f38c3
to
b27ae88
Compare
This looks amazing, let's get it through! |
64e7136
to
71517d5
Compare
71517d5
to
c8e89a0
Compare
c8e89a0
to
276f614
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few little nits/questions, but in general this LGTM! Excited to see these changes!!
endpoint: `${resourceEndpoint}?q=${queryParams}`, | ||
}); | ||
|
||
return json?.result?.map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still get a little excited when I see or use these operators.
Impacts #8976 |
SUMMARY
/api/v1/database/schemas
for fetching list of schemas.LIST_VIEWS_SIP34_FILTER_UI
feature flagsrc/views/*
->src/views/CRUD/*
(eg,src/views/datasetList/DatasetList.tsx
->src/views/CRUD/dataset/DatasetList.tsx
) -- idea being thatsrc/views/CRUD
namespace can share code, ie, data fetching logic, state hooks, edit modals, delete modals, etcBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION
ENABLE_REACT_CRUD_VIEWS
flag