-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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(explore): remove unnecessary parameters from the explore url #17123
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17123 +/- ##
==========================================
+ Coverage 76.94% 76.98% +0.03%
==========================================
Files 1031 1031
Lines 55213 55500 +287
Branches 7505 7629 +124
==========================================
+ Hits 42483 42726 +243
- Misses 12480 12524 +44
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
Outdated
Show resolved
Hide resolved
/testenv up |
@pkdotson Ephemeral environment spinning up at http://54.214.199.175:8080. Credentials are |
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DASHBOARD_NATIVE_FILTERS_SET=true FEATURE_DASHBOARD_FILTERS_EXPERIMENTAL=true |
@pkdotson Ephemeral environment spinning up at http://34.212.20.21:8080. Credentials are |
if we know these parameters are from dashboard, why not |
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.
Tested in eph env and looks good to go! Thanks for the fix.
It's a small difference, but it seems cleaner to me to have the url builder function take a full formData object and decide on its own what should go in the url. |
Ephemeral environment shutdown and build artifacts deleted. |
…che#17123) * remove unnecessary parameters from the explore url * refactor, test
) * remove unnecessary parameters from the explore url * refactor, test (cherry picked from commit 57f869c)
SUMMARY
The url was getting too long from including all this extra dashboard state that isn't used on the explore page. Now the extra state will be filtered out.
Truly solving the long explore url issue will require more substantial changes, but this will make it less likely for now.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION