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
fix: choose language link for local dev #9215
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9215 +/- ##
=======================================
Coverage 58.91% 58.91%
=======================================
Files 372 372
Lines 11996 11996
Branches 2937 2937
=======================================
Hits 7068 7068
Misses 4750 4750
Partials 178 178
Continue to review full report at Codecov.
|
@rusackas this looks like a nice fix, might be up your alley for a review |
d29e8a3
to
27af09d
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, assuming the urls are always consistent.
@@ -67,7 +67,7 @@ export default function setupApp() { | |||
) { | |||
ev.preventDefault(); | |||
SupersetClient.get({ | |||
endpoint: ev.currentTarget.href, | |||
url: ev.currentTarget.href, |
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.
Are we sure that the url will always be a full url instead of a relative one?
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.
Even if it's not, SupersetClient
should still be able to handle it. fetch
does accept relative URLs.
@superset-ui/connection will add protocol and host to an "endpoint" call. `e.currentTarget.href` may return the full URL instead of the relative url.
27af09d
to
b3854af
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.
seems legit, nice fix!
CATEGORY
SUMMARY
@superset-ui/connection will add protocol and host to an "endpoint" call.
e.currentTarget.href
may return the full URL instead of the relative url. This sometimes results in invalid URLs for change language API calls.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
In local dev environment, change language will result in a 404 error:
The result should be gone after this fix.
TEST PLAN
Change language in local dev box will work and should not throw an error in the browser console.
ADDITIONAL INFORMATION
N/A
REVIEWERS
@kristw