-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
[SIP-4] replace dashboard ajax calls with SupersetClient
#5854
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5854 +/- ##
=======================================
Coverage 77.28% 77.28%
=======================================
Files 47 47
Lines 9332 9332
=======================================
Hits 7212 7212
Misses 2120 2120 Continue to review full report at Codecov.
|
8cd1b07
to
1b314b4
Compare
1b314b4
to
e071734
Compare
I do have some concerns for this PR:
I guess all above are somewhat discussed in the meeting. If all of our major contributors (especially frontend contributors) accept inconvenience, i can accept it too :) @mistercrunch @betodealmeida @hughhhh @xtinec @JamshedRahman @jeffreythewang @fabianmenges @michellethomas @williaster @kristw @conglei |
good comments, @graceguo-supercat. Several good questions in there, I added my response to the actual SIP-4 here. |
e071734
to
c389e77
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.
LGTM
* [core] replace dashboard ajax calls with SupersetClient * [core] fix SupersetClient dashboard tests * [dashboard][superset-client] don't error by parsing save dashboard response as json
This PR is one of a few PRs that implement the final step 4) discussed in #5772, to refactor just dashboard-specific ajax calls (not including charts) for easier review
Note that the new
@superset-ui/core
dep + setup forSupersetClient
is duplicated across all ajax PRs:SupersetClient
#5869SupersetClient
#5875SupersetClient
#5896@kristw @mistercrunch @graceguo-supercat @michellethomas @conglei