You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "geido (via GitHub)" <gi...@apache.org> on 2023/04/25 13:13:14 UTC
[GitHub] [superset] geido opened a new pull request, #23811: fix: Hide FiltersPanel in standalone mode 3
geido opened a new pull request, #23811:
URL: https://github.com/apache/superset/pull/23811
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
This PR https://github.com/apache/superset/pull/23543 fixed an issue for which reports would have to render but not display. However, it failed to hide the FiltersPanel (wrapper of the vertical FilterBar) that gets a fixed width and shows as a blank space in reports.
### BEFORE
### TESTING INSTRUCTIONS
1. Visit a Dashboard with existing filters
2. Enter reports standalone mode with standalone=3 in the url
3. Make sure the Dashboard loads while the FilterBar is hidden fully and there is no blank space
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] 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] geido commented on a diff in pull request #23811: fix: Hide FiltersPanel in standalone mode 3
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #23811:
URL: https://github.com/apache/superset/pull/23811#discussion_r1176781952
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -654,6 +655,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
return (
<FiltersPanel
width={filterBarWidth}
+ hidden={isReport}
Review Comment:
The prop itself it is still required for the component as it can be included directly like is the case of the horizontal FilterBar. In this specific case, as the wrapper is hidden too, it's not necessary to pass it to the FilterBar most probably but I think there is no harm in having it there either
--
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 #23811: fix: Hide FiltersPanel in standalone mode 3
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23811:
URL: https://github.com/apache/superset/pull/23811#issuecomment-1521775345
## [Codecov](https://codecov.io/gh/apache/superset/pull/23811?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 [#23811](https://codecov.io/gh/apache/superset/pull/23811?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4ae9b63) into [master](https://codecov.io/gh/apache/superset/commit/742c2cd8fb13090a858f4f35088b4113c009a6b5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (742c2cd) will **decrease** coverage by `11.40%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #23811 +/- ##
===========================================
- Coverage 67.98% 56.58% -11.40%
===========================================
Files 1936 1936
Lines 74928 74927 -1
Branches 8140 8140
===========================================
- Hits 50942 42401 -8541
- Misses 21894 30434 +8540
Partials 2092 2092
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.92% <ø> (ø)` | |
| python | `59.12% <ø> (-23.56%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `52.81% <ø> (ø)` | |
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/23811?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...d/components/DashboardBuilder/DashboardBuilder.tsx](https://codecov.io/gh/apache/superset/pull/23811?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=) | `73.73% <ø> (-0.27%)` | :arrow_down: |
| [...board/components/nativeFilters/FilterBar/index.tsx](https://codecov.io/gh/apache/superset/pull/23811?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==) | `50.53% <ø> (ø)` | |
... and [307 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23811/indirect-changes?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] geido commented on a diff in pull request #23811: fix: Hide FiltersPanel in standalone mode 3
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #23811:
URL: https://github.com/apache/superset/pull/23811#discussion_r1176781952
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -654,6 +655,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
return (
<FiltersPanel
width={filterBarWidth}
+ hidden={isReport}
Review Comment:
The prop itself it is still required for the component as it can be included directly like is the case of the horizontal FilterBar. In this specific case, as the wrapper is hidden too, it's not necessary to pass it to the FilterBar too most probably but I think there is no harm in having it there either
--
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 #23811: fix: Hide FiltersPanel in standalone mode 3
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #23811:
URL: https://github.com/apache/superset/pull/23811#discussion_r1176789260
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -654,6 +655,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
return (
<FiltersPanel
width={filterBarWidth}
+ hidden={isReport}
Review Comment:
Removed. Looks good
--
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 #23811: fix: Hide FiltersPanel in standalone mode 3
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #23811:
URL: https://github.com/apache/superset/pull/23811
--
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 #23811: fix: Hide FiltersPanel in standalone mode 3
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #23811:
URL: https://github.com/apache/superset/pull/23811#discussion_r1176786089
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -654,6 +655,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
return (
<FiltersPanel
width={filterBarWidth}
+ hidden={isReport}
Review Comment:
Let me quickly test and remove it if it causes no harm
--
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 #23811: fix: Hide FiltersPanel in standalone mode 3
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #23811:
URL: https://github.com/apache/superset/pull/23811#discussion_r1176780017
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -654,6 +655,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
return (
<FiltersPanel
width={filterBarWidth}
+ hidden={isReport}
Review Comment:
Do we still need `hidden` in line 670?
--
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