You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/06/04 14:57:17 UTC
[GitHub] [superset] michael-s-molina opened a new pull request #14987: Fix: Aligns top level tabs when native filters are enabled
michael-s-molina opened a new pull request #14987:
URL: https://github.com/apache/superset/pull/14987
### SUMMARY
Fixes https://github.com/apache/superset/issues/14973
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
See the original issue for before screenshots.
https://user-images.githubusercontent.com/70410625/120821221-a6584380-c52b-11eb-82fa-40c3868c7631.mp4
@villebro @junlincc
### TESTING INSTRUCTIONS
1 - Enable the native filters
2 - Open a dashboard with top-level tabs
3 - Expand the native filters
4 - The top-level tabs should be aligned with the dashboard content
### ADDITIONAL INFORMATION
- [x] Has associated issue:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a change in pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #14987:
URL: https://github.com/apache/superset/pull/14987#discussion_r646553411
##########
File path: superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
##########
@@ -161,6 +162,11 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
(hideDashboardHeader ? 0 : HEADER_HEIGHT) +
(topLevelTabs ? TABS_HEIGHT : 0);
+ const tabsPaddingLeft =
+ nativeFiltersEnabled && !editMode && dashboardFiltersOpen
+ ? NATIVE_FILTER_BAR_WIDTH + 20
+ : 8;
Review comment:
Can we use theme variables here? `{theme.gridUnit * 2}px`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
suddjian commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-856130399
@michael-s-molina I agree. The design with the filters outside of tabs better communicates to the user how the filters interact with the rest of the application, so we should do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-856168012
I'm closing this PR and opening a new one with the proposed solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-856157847
@michael-s-molina Hi Michael, dashboard top right tab and dashboard are not aligned perfectly especially on resize. could we address in this or the new design pr?
https://user-images.githubusercontent.com/67837651/121069001-d3059880-c781-11eb-8ad2-66768897f7c8.mov
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc edited a comment on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
junlincc edited a comment on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-856141836
@michael-s-molina please proceed this new design, this is much better. 🙏 thank you all for offering suggestions. I think we can merge this PR first though!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-855912300
> > I also think this is the best approach but it seems that we have a requirement from a client asking to nest the filters inside a top-level tab. @suddjian @junlincc can you check if we can change this requirement?
>
> I feel like that's a case that we discussed some time ago - that we can't treat native filters like filter box with slightly different UI. I think that native filters are "global" to the dashboard by design and the panel doesn't belong to a single top level tab. Scoping of some filters to a tab have been addressed by highlighting tabs and hiding filters that are out of scope, so we don't need to nest the native filters panel to indicate that it belongs to some tab. Though let's wait for an opinion from product point of view 🙂 cc @junlincc
The possibility to place native filters on the dashboard layout (similar to Filter Box) is outside the scope of Native Filters - those that want to do that can enable the `DASHBOARD_FILTERS_EXPERIMENTAL` feature flag. While placing a filter component close to charts within scope can feel intuitive, I personally feel the new in/out of scope logic introduced by #14933 provides a better UX and makes it easier to quickly check which filters are available and in scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-856141836
@michael-s-molina please proceed this change, this is much better. 🙏 thank you all for offering suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-855892646
What do you think about making it look like this?
I'm not sure if I like that empty space on the left side of the bar with top level tabs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-855895328
> What do you think about making it look like this?
> 
>
> I'm not sure if I like that empty space on the left side of the bar with top level tabs
I also think this is the best approach but it seems that we have a requirement from a client asking to nest the filters inside a top-level tab. @suddjian @junlincc can you check if we can change this requirement?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a change in pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #14987:
URL: https://github.com/apache/superset/pull/14987#discussion_r646554598
##########
File path: superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
##########
@@ -55,6 +55,7 @@ import DashboardContainer from './DashboardContainer';
const TABS_HEIGHT = 47;
const HEADER_HEIGHT = 67;
+const NATIVE_FILTER_BAR_WIDTH = 255;
Review comment:
Why 255 instead of 250?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-854804603
# [Codecov](https://codecov.io/gh/apache/superset/pull/14987?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#14987](https://codecov.io/gh/apache/superset/pull/14987?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (99879fd) into [master](https://codecov.io/gh/apache/superset/commit/6955ed287bcccf5f70c1bc9e10448a57601f7807?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6955ed2) will **increase** coverage by `0.06%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/superset/pull/14987?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #14987 +/- ##
==========================================
+ Coverage 77.61% 77.68% +0.06%
==========================================
Files 965 965
Lines 49504 49542 +38
Branches 6259 6271 +12
==========================================
+ Hits 38425 38489 +64
+ Misses 10879 10851 -28
- Partials 200 202 +2
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `72.64% <75.00%> (+0.15%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/14987?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...nd/src/dashboard/containers/DashboardComponent.jsx](https://codecov.io/gh/apache/superset/pull/14987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZENvbXBvbmVudC5qc3g=) | `92.30% <ø> (ø)` | |
| [...end/src/dashboard/components/StickyVerticalBar.tsx](https://codecov.io/gh/apache/superset/pull/14987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1N0aWNreVZlcnRpY2FsQmFyLnRzeA==) | `46.66% <33.33%> (-3.34%)` | :arrow_down: |
| [...d/components/DashboardBuilder/DashboardBuilder.tsx](https://codecov.io/gh/apache/superset/pull/14987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZEJ1aWxkZXIvRGFzaGJvYXJkQnVpbGRlci50c3g=) | `86.59% <75.00%> (-0.50%)` | :arrow_down: |
| [...d/src/dashboard/components/gridComponents/Tabs.jsx](https://codecov.io/gh/apache/superset/pull/14987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL1RhYnMuanN4) | `87.50% <100.00%> (+0.12%)` | :arrow_up: |
| [...board/components/nativeFilters/FilterBar/index.tsx](https://codecov.io/gh/apache/superset/pull/14987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL2luZGV4LnRzeA==) | `100.00% <100.00%> (ø)` | |
| [...et-frontend/src/dashboard/actions/nativeFilters.ts](https://codecov.io/gh/apache/superset/pull/14987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL25hdGl2ZUZpbHRlcnMudHM=) | `82.69% <0.00%> (+1.84%)` | :arrow_up: |
| [...end/src/filters/components/Range/transformProps.ts](https://codecov.io/gh/apache/superset/pull/14987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS90cmFuc2Zvcm1Qcm9wcy50cw==) | `100.00% <0.00%> (+87.50%)` | :arrow_up: |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/14987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `88.67% <0.00%> (+88.67%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14987?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14987?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6955ed2...99879fd](https://codecov.io/gh/apache/superset/pull/14987?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #14987:
URL: https://github.com/apache/superset/pull/14987#discussion_r646556793
##########
File path: superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
##########
@@ -55,6 +55,7 @@ import DashboardContainer from './DashboardContainer';
const TABS_HEIGHT = 47;
const HEADER_HEIGHT = 67;
+const NATIVE_FILTER_BAR_WIDTH = 255;
Review comment:
To fix the inner margin-right of the filter values. They have a fixed width.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #14987:
URL: https://github.com/apache/superset/pull/14987#issuecomment-855902769
> I also think this is the best approach but it seems that we have a requirement from a client asking to nest the filters inside a top-level tab. @suddjian @junlincc can you check if we can change this requirement?
I feel like that's a case that we discussed some time ago - that we can't treat native filters like filter box with slightly different UI. I think that native filters are "global" to the dashboard by design and the panel doesn't belong to a single top level tab. Scoping of some filters to a tab have been addressed by highlighting tabs and hiding filters that are out of scope, so we don't need to nest the native filters panel to indicate that it belongs to some tab. Though let's wait for an opinion from product point of view 🙂 cc @junlincc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a change in pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #14987:
URL: https://github.com/apache/superset/pull/14987#discussion_r646557313
##########
File path: superset-frontend/src/dashboard/components/StickyVerticalBar.tsx
##########
@@ -79,6 +80,7 @@ export const StickyVerticalBar: React.FC<SVBProps> = ({
distanceFromTop: number;
}) => (
<Contents
+ id="sticky-vertical-bar"
Review comment:
Do we need that id here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina closed pull request #14987: fix: Aligns top level tabs when native filters are enabled
Posted by GitBox <gi...@apache.org>.
michael-s-molina closed pull request #14987:
URL: https://github.com/apache/superset/pull/14987
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org