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 2023/01/19 16:30:11 UTC
[GitHub] [superset] geido opened a new pull request, #22785: feat: Move cross filters to Dashboard
geido opened a new pull request, #22785:
URL: https://github.com/apache/superset/pull/22785
### SUMMARY
This PR does the following:
- Removes the ability to emit cross filters from Explore
- Moves the emission of cross filters at the Dashboard level
- Removes the cross filter target name introduced in https://github.com/apache/superset/issues/13966
- Moves the filter bar location settings in a submenu
### AFTER
https://user-images.githubusercontent.com/60598000/213498463-4b990134-247b-4033-9061-cbd617e7be13.mp4
### TESTING INSTRUCTIONS
1. Go to a Dashboard
2. Open the filter bar on the left
3. Click on the settings icon
4. Make sure the checkbox to enable/disable cross filters is appearing
5. Verify that cross filters are enabled by default (requires Feature Flag DASHBOARD_CROSS_FILTERS)
7. Disable cross filtering and make sure the change takes effect
8. Verify that the filter bar orientation settings are appearing in a submenu and working as intended
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [x] Required feature flags: DASHBOARD_CROSS_FILTERS
- [ ] 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1400236660
Probably not related to this PR but clicking on a search icon if filter indicator (which is supposed to highlight the cross filter source chart) grays out everything, including the source chart, and you can't do anything except for reloading the page
https://user-images.githubusercontent.com/15073128/214035830-e49dca5b-8ca5-4ab3-bcd3-7a569f6707bb.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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] geido commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1400763225
Thanks @kgabryje both bugs exist in master. I will create separate tickets for those.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1397275823
# [Codecov](https://codecov.io/gh/apache/superset/pull/22785?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 [#22785](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f139e76) into [master](https://codecov.io/gh/apache/superset/commit/5a422b31c5d1ee3059c7659b376e3b8e1eeb257b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a422b3) will **decrease** coverage by `1.03%`.
> The diff coverage is `63.63%`.
> :exclamation: Current head f139e76 differs from pull request most recent head ee3d081. Consider uploading reports for the commit ee3d081 to get more accurate results
```diff
@@ Coverage Diff @@
## master #22785 +/- ##
==========================================
- Coverage 67.09% 66.06% -1.03%
==========================================
Files 1869 1869
Lines 71597 72417 +820
Branches 7821 7821
==========================================
- Hits 48035 47844 -191
- Misses 21534 22545 +1011
Partials 2028 2028
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.57% <50.00%> (+0.07%)` | :arrow_up: |
| python | `78.90% <100.00%> (-2.44%)` | :arrow_down: |
| sqlite | `76.47% <100.00%> (+<0.01%)` | :arrow_up: |
| unit | `51.48% <50.00%> (-0.01%)` | :arrow_down: |
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/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...t-controls/src/sections/echartsTimeSeriesQuery.tsx](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NlY3Rpb25zL2VjaGFydHNUaW1lU2VyaWVzUXVlcnkudHN4) | `100.00% <ø> (ø)` | |
| [...onents/ColumnConfigControl/ColumnConfigControl.tsx](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9jb21wb25lbnRzL0NvbHVtbkNvbmZpZ0NvbnRyb2wvQ29sdW1uQ29uZmlnQ29udHJvbC50c3g=) | `2.17% <ø> (ø)` | |
| [...trols/components/ColumnConfigControl/constants.tsx](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9jb21wb25lbnRzL0NvbHVtbkNvbmZpZ0NvbnRyb2wvY29uc3RhbnRzLnRzeA==) | `100.00% <ø> (ø)` | |
| [...rt-controls/src/shared-controls/customControls.tsx](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9jdXN0b21Db250cm9scy50c3g=) | `29.41% <ø> (ø)` | |
| [...lugin-chart-echarts/src/BoxPlot/EchartsBoxPlot.tsx](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQm94UGxvdC9FY2hhcnRzQm94UGxvdC50c3g=) | `0.00% <0.00%> (ø)` | |
| [...s/plugin-chart-echarts/src/BoxPlot/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQm94UGxvdC9jb250cm9sUGFuZWwudHM=) | `5.55% <ø> (ø)` | |
| [.../plugins/plugin-chart-echarts/src/BoxPlot/types.ts](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQm94UGxvdC90eXBlcy50cw==) | `0.00% <ø> (ø)` | |
| [.../plugin-chart-echarts/src/Funnel/EchartsFunnel.tsx](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvRnVubmVsL0VjaGFydHNGdW5uZWwudHN4) | `0.00% <0.00%> (ø)` | |
| [...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvRnVubmVsL2NvbnRyb2xQYW5lbC50c3g=) | `66.66% <ø> (ø)` | |
| [...d/plugins/plugin-chart-echarts/src/Funnel/types.ts](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvRnVubmVsL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| ... and [154 more](https://codecov.io/gh/apache/superset/pull/22785?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] cccs-joel commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by "cccs-joel (via GitHub)" <gi...@apache.org>.
cccs-joel commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1560123148
> Thanks @kgabryje both bugs exist in master. I will create separate tickets for those.
Hi @kgabryje, can you link these tickets 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1400243836
Another bug, not sure if related to this PR:
1. Create a new dashboard
2. Add some charts
3. Save
4. Try to apply cross filter
5. There are no filter indicators in target charts and the source chart also filters itself (it shouldn't) - see screenshot
6. Refresh the page - you're back in the edit mode (probably shouldn't?). If you click discard and try to apply cross filter, now it works as expected
<img width="1792" alt="image" src="https://user-images.githubusercontent.com/15073128/214036796-88b75b15-e98d-4daf-97e0-b0f38e25a731.png">
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1400144293
/testev up FEATURE_DASHBOARD_CROSS_FILTERS=true
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] github-actions[bot] commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by github-actions.
github-actions[bot] commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1401837241
@kgabryje Ephemeral environment spinning up at http://35.87.36.211:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1397411037
This is a great improvement, happy to see the control moving from Explore to Dashboard!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1401952595
There might be a few small styling issues around the checkbox:
1. I think it's too big - 18x18px vs 16x16 from the designs
2. It's not centered (see screenshot)
3. I think the spacing between checkbox and text is too small
<img width="431" alt="image" src="https://user-images.githubusercontent.com/15073128/214307426-1e29476f-e9b2-4eff-aab5-69208edcc8f7.png">
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1400144577
/testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 diff in pull request #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #22785:
URL: https://github.com/apache/superset/pull/22785#discussion_r1085211303
##########
superset-frontend/src/dashboard/util/crossFilters.ts:
##########
@@ -0,0 +1,28 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+
+export const isCrossFiltersEnabled = (
+ metadataCrossFiltersEnabled: boolean | undefined,
+): boolean =>
+ (isFeatureEnabled(FeatureFlag.DASHBOARD_CROSS_FILTERS) &&
+ (metadataCrossFiltersEnabled === undefined ||
+ metadataCrossFiltersEnabled)) ||
+ false;
Review Comment:
I think `|| false` might be redundant
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] geido commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1402349387
> <img alt="image" width="231" src="https://user-images.githubusercontent.com/15073128/214364126-3930a97c-005f-4189-9b25-eaff162a5b05.png">
>
> The spacing's missing now :(
@kgabryje are you able to reproduce that consistently? I am not able to reproduce it locally
<img width="863" alt="Screenshot 2023-01-24 at 18 50 08" src="https://user-images.githubusercontent.com/60598000/214369635-e1049291-d8d2-40c1-9f24-44d9356e47d3.png">
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] geido merged pull request #22785: feat: Move cross filters to Dashboard
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #22785:
URL: https://github.com/apache/superset/pull/22785
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] github-actions[bot] commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by github-actions.
github-actions[bot] commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1400146950
@kgabryje Ephemeral environment spinning up at http://34.222.33.161:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 diff in pull request #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #22785:
URL: https://github.com/apache/superset/pull/22785#discussion_r1083969592
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx:
##########
@@ -116,7 +116,7 @@ const Header: FC<HeaderProps> = ({ toggleFiltersBar }) => {
<Wrapper>
<TitleArea>
<span>{t('Filters')}</span>
- {canSetHorizontalFilterBar && <FilterBarOrientationSelect />}
+ {canSetHorizontalFilterBar && <FilterBarSettings />}
Review Comment:
The gear icon is not visible if `HORIZONTAL_FILTER_BAR` is disabled.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] geido commented on a diff in pull request #22785: feat: Move cross filters to Dashboard
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #22785:
URL: https://github.com/apache/superset/pull/22785#discussion_r1084306083
##########
superset-frontend/src/components/DropdownSelectableIcon/index.tsx:
##########
@@ -86,24 +101,40 @@ export default (props: DropDownSelectableProps) => {
{info}
</div>
)}
- {menuItems.map(m => (
- <Menu.Item key={m.key}>
- {m.label}
- {selectedKeys?.includes(m.key) && (
- <Icons.Check iconColor={theme.colors.primary.base} />
- )}
- </Menu.Item>
- ))}
+ {menuItems.map(m =>
+ m.children?.length ? (
+ <SubMenu
+ title={m.label}
+ key={m.key}
+ data-test="dropdown-selectable-icon-submenu"
+ >
+ {m.children.map(s => menuItem(s.label, s.key))}
+ </SubMenu>
+ ) : (
+ menuItem(m.label, m.key)
+ ),
+ )}
</StyledMenu>
),
[info, menuItems],
);
return (
- <StyledDropdownButton
- overlay={overlayMenu}
- trigger={['click']}
- icon={icon}
- />
+ <>
+ <Global
+ styles={css`
+ .ant-dropdown-menu .ant-dropdown-menu-item > .tick-menu-item {
+ float: right;
+ margin-right: 0;
+ font-size: ${theme.typography.sizes.xl}px;
Review Comment:
Great tip!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] geido commented on a diff in pull request #22785: feat: Move cross filters to Dashboard
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #22785:
URL: https://github.com/apache/superset/pull/22785#discussion_r1084321084
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx:
##########
@@ -116,7 +116,7 @@ const Header: FC<HeaderProps> = ({ toggleFiltersBar }) => {
<Wrapper>
<TitleArea>
<span>{t('Filters')}</span>
- {canSetHorizontalFilterBar && <FilterBarOrientationSelect />}
+ {canSetHorizontalFilterBar && <FilterBarSettings />}
Review Comment:
Great catch
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] github-actions[bot] commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by github-actions.
github-actions[bot] commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1403936651
Ephemeral environment shutdown and build artifacts deleted.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1401832159
/testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] github-actions[bot] commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by github-actions.
github-actions[bot] commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1402266770
@kgabryje Ephemeral environment spinning up at http://52.40.81.25:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1402260314
/testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 diff in pull request #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #22785:
URL: https://github.com/apache/superset/pull/22785#discussion_r1083921413
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx:
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useCallback, useMemo, useState } from 'react';
Review Comment:
I see that this file based on `FilterBarOrientationSelect` file. Could you use `git mv` to rename it so that git knows that it's actually the same file but renamed and shows the diff? It would make reviewing easier
##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -77,6 +77,8 @@ const propTypes = {
postTransformProps: PropTypes.func,
datasetsStatus: PropTypes.oneOf(['loading', 'error', 'complete']),
isInView: PropTypes.bool,
+ // cross-filters
Review Comment:
Can we remove that comment?
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -177,3 +186,48 @@ export function saveFilterBarOrientation(orientation: FilterBarOrientation) {
}
};
}
+
+export function saveCrossFiltersSetting(crossFiltersEnabled: boolean) {
Review Comment:
This function is pretty similar to `saveFilterBarOrientation`. I'm wondering if we could DRY it a bit 🤔
##########
superset-frontend/src/dashboard/actions/hydrate.js:
##########
@@ -394,6 +394,11 @@ export const hydrateDashboard =
const { roles } = user;
const canEdit = canUserEditDashboard(dashboard, user);
+ const crossFiltersEnabled =
+ (isFeatureEnabled(FeatureFlag.DASHBOARD_CROSS_FILTERS) &&
+ (metadata.cross_filters_enabled === undefined ||
Review Comment:
Nit: We have this logic in 2 places (true if undefined), maybe we could move it to a utility function?
##########
superset-frontend/src/components/DropdownSelectableIcon/index.tsx:
##########
@@ -86,24 +101,40 @@ export default (props: DropDownSelectableProps) => {
{info}
</div>
)}
- {menuItems.map(m => (
- <Menu.Item key={m.key}>
- {m.label}
- {selectedKeys?.includes(m.key) && (
- <Icons.Check iconColor={theme.colors.primary.base} />
- )}
- </Menu.Item>
- ))}
+ {menuItems.map(m =>
+ m.children?.length ? (
+ <SubMenu
+ title={m.label}
+ key={m.key}
+ data-test="dropdown-selectable-icon-submenu"
+ >
+ {m.children.map(s => menuItem(s.label, s.key))}
+ </SubMenu>
+ ) : (
+ menuItem(m.label, m.key)
+ ),
+ )}
</StyledMenu>
),
[info, menuItems],
);
return (
- <StyledDropdownButton
- overlay={overlayMenu}
- trigger={['click']}
- icon={icon}
- />
+ <>
+ <Global
+ styles={css`
+ .ant-dropdown-menu .ant-dropdown-menu-item > .tick-menu-item {
+ float: right;
+ margin-right: 0;
+ font-size: ${theme.typography.sizes.xl}px;
Review Comment:
1. I think we could use Icon's property `iconSize="xl"` instead of `font-size`
2. Maybe we could wrap icon and label in a flex div instead of using `float: right`? That would also allow us not to use `<Global />`
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -62,6 +62,8 @@ const propTypes = {
ownState: PropTypes.object,
postTransformProps: PropTypes.func,
source: PropTypes.oneOf([ChartSource.Dashboard, ChartSource.Explore]),
+ // cross filters
Review Comment:
And that one 😛
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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 #22785: feat: Move cross filters to Dashboard
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1402316213
<img width="231" alt="image" src="https://user-images.githubusercontent.com/15073128/214364126-3930a97c-005f-4189-9b25-eaff162a5b05.png">
The spacing's missing now :(
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] geido commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1402380855
> <img alt="image" width="231" src="https://user-images.githubusercontent.com/15073128/214364126-3930a97c-005f-4189-9b25-eaff162a5b05.png">
>
> The spacing's missing now :(
Nice catch. This happening when the FF HORIZONTAL_FILTER_BAR is off which was always on in my environment. It should be fixed now.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] cccs-joel commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by "cccs-joel (via GitHub)" <gi...@apache.org>.
cccs-joel commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1560124938
> Thanks @kgabryje both bugs exist in master. I will create separate tickets for those.
Hi @kgabryje, can you link these tickets 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] cccs-joel commented on pull request #22785: feat: Move cross filters to Dashboard
Posted by "cccs-joel (via GitHub)" <gi...@apache.org>.
cccs-joel commented on PR #22785:
URL: https://github.com/apache/superset/pull/22785#issuecomment-1584673480
@kgabryje ^
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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