You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "LevisNgigi (via GitHub)" <gi...@apache.org> on 2025/03/25 08:14:44 UTC
[PR] fix:Fix bug where dashboard did not enter fullscreen mode. [superset]
LevisNgigi opened a new pull request, #32839:
URL: https://github.com/apache/superset/pull/32839
<!---
fix(dashboard): Toggle fullscreen parameter correctly
-->
### SUMMARY
<!--- This PR fixes an issue with the dashboard's fullscreen mode. Previously, when clicking "Enter fullscreen" from the dashboard menu, the standalone parameter was not being properly added to the URL, preventing the dashboard from entering fullscreen mode correctly. The fix properly toggles the standalone parameter between null and 1 when the fullscreen option is selected. -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
1) Open any dashboard
2) Click on the menu in the top right corner (three dots)
3) Click "Enter fullscreen"
4) Verify that the dashboard enters fullscreen mode and the menu option changes to "Exit fullscreen"
5) Click "Exit fullscreen" and verify it returns to normal mode
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x ] Has associated issue: Fixes #32760
- [ ] 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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32839:
URL: https://github.com/apache/superset/pull/32839#discussion_r2011553006
##########
superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:
##########
@@ -97,11 +97,13 @@ export const useHeaderActionsMenu = ({
showPropertiesModal();
break;
case MenuKeys.ToggleFullscreen: {
+ const isCurrentlyStandalone =
+ getUrlParam(URL_PARAMS.standalone) === 1;
Review Comment:
### Invalid Type Comparison <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Direct comparison with number 1 is incorrect as getUrlParam likely returns a string, not a number.
###### Why this matters
This type coercion can lead to unexpected behavior when comparing string '1' with number 1 using strict equality (===). The comparison will always be false even when the parameter is '1'.
###### Suggested change ∙ *Feature Preview*
Use string comparison or explicit conversion:
```typescript
const isCurrentlyStandalone = String(getUrlParam(URL_PARAMS.standalone)) === '1';
```
or
```typescript
const isCurrentlyStandalone = Number(getUrlParam(URL_PARAMS.standalone)) === 1;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/74039c5e-3a9d-4837-a3d5-14261deaf7da/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/74039c5e-3a9d-4837-a3d5-14261deaf7da?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/74039c5e-3a9d-4837-a3d5-14261deaf7da?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/74039c5e-3a9d-4837-a3d5-14261deaf7da?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/74039c5e-3a9d-4837-a3d5-14261deaf7da)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:96185117-a268-445b-937a-0fcf7ebfbcea -->
[](96185117-a268-445b-937a-0fcf7ebfbcea)
--
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
Re: [PR] fix:Fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2750434812
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](49864bf6-dcec-4bc0-84ac-88e6ea2dedfe)
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2758595307
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/32839?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 65.78%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`f7ab6fa`)](https://app.codecov.io/gh/apache/superset/commit/f7ab6fa21b2fe82e6d0479f3f1533e9940c94372?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1688 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #32839 +/- ##
==========================================
+ Coverage 60.48% 65.78% +5.29%
==========================================
Files 1931 549 -1382
Lines 76236 39467 -36769
Branches 8568 0 -8568
==========================================
- Hits 46114 25963 -20151
+ Misses 28017 13504 -14513
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/32839/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/32839/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/32839?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2772424063
> @LevisNgigi would you please rebase your PR with master? Thank you!
@geido I have rebased.
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2758718395
Glad we added a test for this, but wondering if there should be one where if we are in full screen mode that the header bar isn't showing up or even if when in full screen, we should see the exit full screen option.
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2758883283
> Glad we added a test for this, but wondering if there should be one where if we are in full screen mode that the header bar isn't showing up or even if when in full screen, we should see the exit full screen option.
@sadpandajoe ooh yeah we need that will be adding it.
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2772183246
@LevisNgigi would you please rebase your PR with master? Thank you!
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #32839:
URL: https://github.com/apache/superset/pull/32839#discussion_r2021705381
##########
superset-frontend/src/dashboard/components/Header/Header.test.tsx:
##########
@@ -442,6 +442,27 @@ test('should NOT render MetadataBar when embedded', () => {
).not.toBeInTheDocument();
});
+test('should hide edit button when in fullscreen mode', () => {
+ const fullscreenState = {
+ ...initialState,
+ dashboardState: {
+ ...initialState.dashboardState,
+ isFullscreenMode: true,
+ },
+ };
+
+ setup(fullscreenState);
+ expect(screen.queryByTestId('edit-dashboard-button')).not.toBeInTheDocument();
+ expect(screen.getByTestId('actions-trigger')).toBeInTheDocument();
Review Comment:
Should we add Exit fullscreen to be in document and also verify that the main navigation bar isn't displayed 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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2758594668
@sadpandajoe Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/14111307354). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2758628453
@sadpandajoe Ephemeral environment spinning up at http://54.148.40.3: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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2770409373
Seems it's a flaky test?Same test passes locally.
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32839:
URL: https://github.com/apache/superset/pull/32839#issuecomment-2767250281
@geido @sadpandajoe Can we merge this?
--
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
Re: [PR] fix: fix bug where dashboard did not enter fullscreen mode. [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #32839:
URL: https://github.com/apache/superset/pull/32839
--
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