-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
refactor: icon to icons for nativeFilter components #15528
Conversation
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.
looks good to me!
Codecov Report
@@ Coverage Diff @@
## master #15528 +/- ##
==========================================
- Coverage 77.24% 76.95% -0.30%
==========================================
Files 975 976 +1
Lines 50866 51289 +423
Branches 6740 6907 +167
==========================================
+ Hits 39293 39468 +175
- Misses 11357 11602 +245
- Partials 216 219 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
...end/src/dashboard/components/nativeFilters/FilterBar/CascadeFilters/CascadePopover/index.tsx
Outdated
Show resolved
Hide resolved
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 with some minor improvements
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx
Show resolved
Hide resolved
...end/src/dashboard/components/nativeFilters/FilterBar/CascadeFilters/CascadePopover/index.tsx
Outdated
Show resolved
Hide resolved
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.
looks good with @geido 's edits
…rBar/CascadeFilters/CascadePopover/index.tsx Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
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
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true |
@pkdotson Ephemeral environment spinning up at http://34.221.155.212: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.
LGTM!
Ephemeral environment shutdown and build artifacts deleted. |
* initial commit for cascade filters * migrate delete icon * Update superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CascadeFilters/CascadePopover/index.tsx Co-authored-by: Geido <60598000+geido@users.noreply.github.com> * add uniform style for icons * fix lint Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
* initial commit for cascade filters * migrate delete icon * Update superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CascadeFilters/CascadePopover/index.tsx Co-authored-by: Geido <60598000+geido@users.noreply.github.com> * add uniform style for icons * fix lint Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
* initial commit for cascade filters * migrate delete icon * Update superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CascadeFilters/CascadePopover/index.tsx Co-authored-by: Geido <60598000+geido@users.noreply.github.com> * add uniform style for icons * fix lint Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
SUMMARY
This pr migrates the icons for native filters to the new icons component. The delete icon in the filter config modal and the icons in the cross filter componets.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Test in the filterconfig modal and the crossfilter propover
ADDITIONAL INFORMATION