-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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-36] Migrate setupApp.js to setupApp.ts #9180
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9180 +/- ##
=========================================
- Coverage 59.13% 59.1% -0.03%
=========================================
Files 372 372
Lines 11938 11943 +5
Branches 2925 2925
=========================================
Hits 7059 7059
- Misses 4699 4706 +7
+ Partials 180 178 -2
Continue to review full report at Codecov.
|
Since you are making migration to ts, could you add unit test for every new component? The overall test coverage seems keep going down. |
That's a good point, any thoughts about how to test this file? The dependence on jquery makes me a bit uncertain about how to test.... |
07d6b7e
to
1dfacb7
Compare
i want to remove jQuery usage from superset-frontend... is it possible? https://www.sitepoint.com/jquery-document-ready-plain-javascript/ |
I reimplemented the menu in react, so it's possible to fully deprecate jquery. The react menu hasn't been propagated to all the pages, yet. I was hoping to progressively roll it out by replacing all the server rendered pages, however if someone wants to speed up jquery deprecation that's available. |
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, +1 on jquery deprecation
++ on jquery removal as well. I don't think that's in scope for this PR though... |
CATEGORY
Choose one
SUMMARY
This is an example PR for migrating a JS file to TS. I performed the following steps:
setupApp.js
=>setupApp.ts
npm install @types/jquery
or add// @ts-ignore
if no valid typedefs existTEST PLAN
CI, confirm that previous behavior is still correct. In this case, check changing locales still works and that checkboxes work in CRUD Views
ADDITIONAL INFORMATION
REVIEWERS
to: @nytai @graceguo-supercat @kristw @rusackas