-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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(native-filters): Fix required filters #15572
fix(native-filters): Fix required filters #15572
Conversation
� Conflicts: � superset-frontend/src/dashboard/util/getPermissions.ts
Codecov Report
@@ Coverage Diff @@
## master #15572 +/- ##
==========================================
- Coverage 76.95% 76.93% -0.02%
==========================================
Files 976 976
Lines 51290 51309 +19
Branches 6907 6920 +13
==========================================
+ Hits 39468 39474 +6
- Misses 11603 11612 +9
- Partials 219 223 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true |
@michael-s-molina Ephemeral environment spinning up at http://54.201.111.35:8080. Credentials are |
Bug 1: Error adding a filter with a default value: screen-recording-2021-07-07-at-22517-pm_tDAvgEeF.mp4Bug 2: Time range filter is not being validated on the filter bar: screen-recording-2021-07-07-at-23003-pm_EF6aocH7.mp4 |
@michael-s-molina Hi thanks for review, I fixed bugs that you found and pushed fix 👍 |
thanks for addressing our comments @simcha90 really appreciate it! |
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true |
@michael-s-molina Ephemeral environment spinning up at http://54.203.224.124:8080. Credentials are |
The "Value is required" is still appearing... screen-recording-2021-07-08-at-83158-am_yB19KQN0.mp4 |
👍 my miss I will look on this on Sunday, thanks! |
Ok @simcha90. Let me know when it's ready for review. |
@michael-s-molina I fixed, also on the way I fixed some other related bugs when tested this functionality, I updated them in description, thanks |
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true |
@michael-s-molina Ephemeral environment spinning up at http://34.214.18.187:8080. Credentials are |
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.
Thank you for the fixes @simcha90
Ephemeral environment shutdown and build artifacts deleted. |
@simcha90 @amitmiran137 really appreciate the quick fix and informing our team for review! 🙏 |
* fix:fix get permission function * fix: filters required state * fix: fix CR notes * fix: removre required message * fix: fix validation state (cherry picked from commit d70ac21)
* fix:fix get permission function * fix: filters required state * fix: fix CR notes * fix: removre required message * fix: fix validation state
* fix:fix get permission function * fix: filters required state * fix: fix CR notes * fix: removre required message * fix: fix validation state
* fix:fix get permission function * fix: filters required state * fix: fix CR notes * fix: removre required message * fix: fix validation state
SUMMARY
This PR fix CR notes for native filters from here: #15506 (comment)
Screen.Recording.2021-07-07.at.17.54.44.mov
On the way fix Range filter that doesn't apply default value in next flow:
a. Create Select Filter
b. Switch it's type to Range Filter
Removes reset form functionality because of 2 reasons:
a. When form saved or closed now it destroys Ant form, so no need reset it
b. It applies reset before form closed, and it can mess some form fields before it closed
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION