You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "venkateshwaracholan (via GitHub)" <gi...@apache.org> on 2026/03/24 07:07:37 UTC
[PR] feat: implement native browser fullscreen for dashboard charts [superset]
venkateshwaracholan opened a new pull request, #38819:
URL: https://github.com/apache/superset/pull/38819
### Summary
This PR implements the native **Browser Fullscreen API** for individual dashboard charts. Previously, "fullscreen" was a CSS-based expansion within the viewport, which caused issues with z-index, tooltip clipping, and browser chrome interference.
By leveraging the `requestFullscreen` API on the `ChartHolder` component, charts now utilize the full hardware display, providing a more immersive experience.
### Key Changes
- **Native Fullscreen Integration**: Replaced simulated expansion with `requestFullscreen()` and `exitFullscreen()` in `SliceHeaderControls`.
- **Tooltip Logic Fix**: Updated `plugin-chart-echarts` to toggle `appendToBody` based on `document.fullscreenElement`. This ensures tooltips render inside the fullscreen DOM tree rather than behind it.
- **Ref Propagation**: Threaded `chartHolderRef` from `ChartHolder` -> `Chart` -> `SliceHeader` -> `SliceHeaderControls` to allow the menu to target the container.
- **Portal Management**: Wrapped `ChartHolder` content in `AntdThemeProvider` with a dynamic `getPopupContainer`. When in fullscreen, AntD Portals (Selects, Dropdowns) are attached to the chart holder instead of `document.body`.
- **Global Styles**: Created `Styles.tsx` to handle the `:fullscreen` pseudo-class layout, ensuring the chart occupies `100vw/100vh` with correct padding and background.
- **Event Handling**: Added a `fullscreenchange` listener to sync the internal `isFullSize` state if a user exits via the `Esc` key.
### Screenshots / GIFs
| Before (Viewport) | After (Native API) |
| :--- | :--- |
| *[Add GIF Here]* | *[Add GIF Here]* |
### Testing Instructions
1. Open any dashboard and locate a chart.
2. Open the chart menu (three dots) and select **"Enter fullscreen"**.
3. Verify the chart fills the entire screen.
4. Hover over elements to ensure tooltips are visible.
5. Exit using the `Esc` key or the "Exit Fullscreen" icon and verify the chart returns to its original grid position.
### Checklist
- [x] I have followed the [Superset Contribution Guidelines](https://github.com/apache/superset/blob/master/CONTRIBUTING.md).
- [x] I have added unit tests for the toggle logic in `SliceHeaderControls.test.tsx`.
- [x] My changes follow the "Sentence case" capitalization guidelines.
- [x] I have verified that code coverage has not decreased.
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r3060621409
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -554,13 +609,36 @@ const SliceHeaderControls = (
});
}
+ useEffect(() => {
+ const handleFullscreenChange = () => {
+ const isChartFullscreen =
+ document.fullscreenElement === props.chartHolderRef?.current;
+
+ if (isChartFullscreen !== Boolean(isFullSize)) {
+ props.handleToggleFullSize();
+ }
+
+ queueChartResize();
Review Comment:
**Suggestion:** The fullscreen listener runs in every chart instance, but `queueChartResize()` is called unconditionally, so a single fullscreen change on one chart triggers one resize event per chart. On dashboards with many charts this causes a burst of redundant global resize events and unnecessary re-layout work. Only queue the resize when this chart's fullscreen state actually changes. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Multiple synthetic resize events per fullscreen toggle.
- ⚠️ Extra chart layout work on dashboards with many charts.
- ⚠️ Potential UI jank on fullscreen enter/exit interactions.
```
</details>
```suggestion
queueChartResize();
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard with multiple charts so each chart renders a `SliceHeader` with
controls as seen in
`superset-frontend/src/dashboard/components/SliceHeader/index.tsx:349-353`, where
`<SliceHeaderControls ... chartHolderRef={chartHolderRef} />` is instantiated once per
chart.
2. For each `<SliceHeaderControls>` instance, React mounts the component and runs the
`useEffect` hook at
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:612-69`, which
registers `document.addEventListener('fullscreenchange', handleFullscreenChange);` — one
listener per chart.
3. Trigger fullscreen for a single chart by opening its header menu and choosing the
fullscreen menu item, which calls `handleMenuClick` and then `requestChartFullscreen()` at
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:210-218`,
causing `chartHolder.requestFullscreen()` and subsequently a single `fullscreenchange`
event on `document`.
4. That single `fullscreenchange` event invokes every registered `handleFullscreenChange`
(one per chart); in each instance, lines `617-621` run and `queueChartResize()` (defined
at `157-161` as `window.dispatchEvent(new Event('resize'))`) is called unconditionally, so
a dashboard with N charts emits N synthetic `resize` events for one fullscreen toggle,
even though only the chart whose fullscreen state actually changed needed a resize.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
**Line:** 619:621
**Comment:**
*Performance: The fullscreen listener runs in every chart instance, but `queueChartResize()` is called unconditionally, so a single fullscreen change on one chart triggers one resize event per chart. On dashboards with many charts this causes a burst of redundant global resize events and unnecessary re-layout work. Only queue the resize when this chart's fullscreen state actually changes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38819&comment_hash=17ac429be14dd42b56b7a160cf40a1c613a3e72e7a8e9de609d03988d7915e6f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38819&comment_hash=17ac429be14dd42b56b7a160cf40a1c613a3e72e7a8e9de609d03988d7915e6f&reaction=dislike'>👎</a>
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r3039701929
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -231,9 +235,34 @@ const SliceHeaderControls = (
// eslint-disable-next-line no-unused-expressions
props.exportPivotCSV?.(props.slice.slice_id);
break;
- case MenuKeys.Fullscreen:
- props.handleToggleFullSize();
+ case MenuKeys.Fullscreen: {
+ const el = props.chartHolderRef?.current;
+
+ if (!document.fullscreenElement) {
+ el?.requestFullscreen?.()
+ .then(() => {
+ props.handleToggleFullSize();
+ // Trigger resize to fit the fullscreen container
+ setTimeout(() => {
+ window.dispatchEvent(new Event('resize'));
+ }, 300);
+ })
Review Comment:
Can we make one of those a single source of truth?
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2981372765
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ [data-test='dashboard-component-chart-holder']:fullscreen {
+ background-color: ${theme.colorBgBase};
+ width: 100vw;
+ height: 100vh;
+ display: flex;
Review Comment:
**Suggestion:** The fullscreen container sets `width: 100vw` and `height: 100vh` while also adding padding, which makes the rendered box larger than the viewport under default `content-box` sizing. This can cause overflow/cropping in fullscreen mode. Add `box-sizing: border-box` so padding is included in the fullscreen dimensions. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Fullscreen chart container can exceed viewport bounds.
- ⚠️ Fullscreen chart content may clip near edges.
```
</details>
```suggestion
box-sizing: border-box;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard chart and trigger **Enter fullscreen** from `SliceHeaderControls`
menu; this executes `el?.requestFullscreen?.()` in
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:243`.
2. On success, `handleToggleFullSize()` is called (`index.tsx:245-247`), setting chart
full-size state through `ChartHolder`
(`superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx:232-234`).
3. While full-size is true, `Global styles={fullscreenStyles(theme)}` is mounted at
`index.tsx:656`, applying `Styles.tsx:22-33` to
`[data-test='dashboard-component-chart-holder']:fullscreen`.
4. In that block, `width: 100vw; height: 100vh; padding: ...` (`Styles.tsx:24-28`) uses
default content-box sizing unless overridden; padding is added outside viewport-sized
content box, causing overflow/cropping risk in fullscreen layouts.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx
**Line:** 26:26
**Comment:**
*Logic Error: The fullscreen container sets `width: 100vw` and `height: 100vh` while also adding padding, which makes the rendered box larger than the viewport under default `content-box` sizing. This can cause overflow/cropping in fullscreen mode. Add `box-sizing: border-box` so padding is included in the fullscreen dimensions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38819&comment_hash=a50bf34ee4906273aef5aadd38e6104c9346e08abdb029867cb20c84b9fd1ca5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38819&comment_hash=a50bf34ee4906273aef5aadd38e6104c9346e08abdb029867cb20c84b9fd1ca5&reaction=dislike'>👎</a>
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2986064187
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ [data-test='dashboard-component-chart-holder']:fullscreen {
+ background-color: ${theme.colorBgBase};
+ width: 100vw;
+ height: 100vh;
+ box-sizing: border-box;
+ display: flex;
+ flex-direction: column;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: visible;
+ position: relative;
+ pointer-events: auto;
+ z-index: ${theme.zIndexPopupBase};
+ opacity: 1;
+ visibility: visible;
+
+ /* Ensure children take up available space */
+ .dashboard-chart,
+ .chart-container,
+ .slice_container,
+ .chart-slice {
+ flex: 1 1 auto;
+ height: 100%;
+ width: 100%;
+ display: flex;
+ flex-direction: column;
+ overflow: visible;
+ }
+
+ /* Portaled components inside the fullscreen layer */
+ .ant-dropdown,
+ .ant-tooltip,
+ .ant-modal-root,
+ .ant-select-dropdown,
+ .ant-popover {
+ z-index: ${theme.zIndexPopupBase + 1};
+ pointer-events: auto;
+ }
+ }
+
+ /* Interaction and Header fixes */
+ [data-test='dashboard-component-chart-holder']:fullscreen * {
+ pointer-events: auto;
+ }
+
+ [data-test='dashboard-component-chart-header'] {
Review Comment:
**Suggestion:** The header z-index fix targets a non-existent test attribute (`dashboard-component-chart-header`), so the style never applies and fullscreen header layering remains broken. Point the selector to the actual header attribute and scope it under the fullscreen chart holder so it applies only in fullscreen mode. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Fullscreen chart header z-index fix never executes.
- ⚠️ Header/menu layering can regress in fullscreen mode.
```
</details>
```suggestion
[data-test='dashboard-component-chart-holder']:fullscreen [data-test='slice-header'] {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard chart and trigger fullscreen from menu path
`SliceHeaderControls/index.tsx:238-245` (`MenuKeys.Fullscreen` calls
`el?.requestFullscreen?.()` and then toggles full size state).
2. Confirm fullscreen global CSS is injected at `SliceHeaderControls/index.tsx:655`
(`{isFullSize && <Global styles={fullscreenStyles(theme)} />}`).
3. Inspect selector in `SliceHeaderControls/Styles.tsx:66-69`; it targets
`[data-test='dashboard-component-chart-header']`, but grep shows no such attribute exists
anywhere else in frontend code.
4. Inspect actual header markup at `SliceHeader/index.tsx:260`; header is rendered as
`data-test="slice-header"`, so the z-index rule never matches any real element.
5. Because this rule never applies, intended header layering fix is effectively dead code
during fullscreen; changing selector to existing `slice-header` under fullscreen holder
makes the rule executable.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx
**Line:** 66:66
**Comment:**
*Logic Error: The header z-index fix targets a non-existent test attribute (`dashboard-component-chart-header`), so the style never applies and fullscreen header layering remains broken. Point the selector to the actual header attribute and scope it under the fullscreen chart holder so it applies only in fullscreen mode.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38819&comment_hash=7674e321abf8a38cdbf09969c278efafb8c5ba40a681991528a693008cd132fe&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38819&comment_hash=7674e321abf8a38cdbf09969c278efafb8c5ba40a681991528a693008cd132fe&reaction=dislike'>👎</a>
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4119399708
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: implement native browser fullscreen for dashboard charts [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4218530566
Thanks for all the work on this, @venkateshwaracholan — this has been a really solid improvement over the CSS-based fullscreen, and the iterations through the review have been great. Picking this up to help push it over the line. Pushed three commits on top of your work:
- b724f77b57 — stabilizes the fullscreen controls: fullscreenchange is the single sync point now, useEffect deps are narrowed, and the header z-index selector targets the real element. Addresses @msyavuz, @JCelento, and codeant's open threads.
- 56df068302 — fixes a Cannot set properties of null (setting 'innerHTML') crash when you hover right after clicking Enter fullscreen. Echarts' _keepShow setTimeout was firing on a tooltip component that setOption({ notMerge: true }) had already disposed. Dispatching hideTip before every setOption clears _lastX/_lastY so the stale closure becomes a no-op.
- c020db0e66 — addresses codeant's latest perf flag. Moved queueChartResize() inside the state-change guard so only the chart actually toggling queues a resize, instead of all N charts on the 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
Re: [PR] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "venkateshwaracholan (via GitHub)" <gi...@apache.org>.
venkateshwaracholan commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2981379290
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
+ [data-test='dashboard-component-chart-holder']:fullscreen {
+ background-color: ${theme.colorBgBase};
+ width: 100vw;
+ height: 100vh;
+ display: flex;
+ flex-direction: column;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: visible;
+ position: relative;
+ pointer-events: auto;
+ z-index: 1000;
+ opacity: 1;
+ visibility: visible;
+
+ /* Ensure children take up available space */
+ .dashboard-chart,
+ .chart-container,
+ .slice_container,
+ .chart-slice {
+ flex: 1 1 auto;
+ height: 100%;
+ width: 100%;
+ display: flex;
+ flex-direction: column;
+ overflow: visible;
+ }
+
+ /* Portaled components inside the fullscreen layer */
+ .ant-dropdown,
+ .ant-tooltip,
+ .ant-modal-root,
+ .ant-select-dropdown,
+ .ant-popover {
+ z-index: 99999;
Review Comment:
Updated this to ${theme.zIndexPopupBase + 1} to ensure popups consistently layer above the fullscreen container using theme tokens.
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2981128904
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
Review Comment:
Is this an outdated comment?
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -94,8 +97,10 @@ const VerticalDotsTrigger = () => {
/>
);
};
+const FULLSCREEN_Z_INDEX = 10001;
Review Comment:
is there an ant design way of doing this? Custom z indexes are a bit troublesome
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
+ [data-test='dashboard-component-chart-holder']:fullscreen {
+ background-color: ${theme.colorBgBase};
+ width: 100vw;
+ height: 100vh;
+ display: flex;
+ flex-direction: column;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: visible;
+ position: relative;
+ pointer-events: auto;
+ z-index: 1000;
+ opacity: 1;
+ visibility: visible;
+
+ /* Ensure children take up available space */
+ .dashboard-chart,
+ .chart-container,
+ .slice_container,
+ .chart-slice {
+ flex: 1 1 auto;
+ height: 100%;
+ width: 100%;
+ display: flex;
+ flex-direction: column;
+ overflow: visible;
+ }
+
+ /* Portaled components inside the fullscreen layer */
+ .ant-dropdown,
+ .ant-tooltip,
+ .ant-modal-root,
+ .ant-select-dropdown,
+ .ant-popover {
+ z-index: 99999;
Review Comment:
ditto
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
+ [data-test='dashboard-component-chart-holder']:fullscreen {
+ background-color: ${theme.colorBgBase};
+ width: 100vw;
+ height: 100vh;
+ display: flex;
+ flex-direction: column;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: visible;
+ position: relative;
+ pointer-events: auto;
+ z-index: 1000;
Review Comment:
Let's check if there is an antd way of doing 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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4115913137
## **Sequence Diagram**
This PR replaces CSS-only chart expansion with native browser fullscreen on the chart holder element. It also updates portal and tooltip behavior so dropdowns and tooltips render correctly inside the fullscreen layer and keeps UI state synced when users exit with Esc.
```mermaid
sequenceDiagram
participant User
participant SliceHeaderControls
participant BrowserFullscreen
participant ChartHolder
participant ChartUI
User->>SliceHeaderControls: Choose Enter fullscreen
SliceHeaderControls->>BrowserFullscreen: Request fullscreen for chart holder
BrowserFullscreen-->>ChartHolder: Activate fullscreen element
SliceHeaderControls->>ChartHolder: Toggle fullsize state and trigger resize
ChartHolder->>ChartUI: Render popups and menus in chart holder when fullscreen
ChartUI->>ChartUI: Use fullscreen aware tooltip container
User->>BrowserFullscreen: Press Esc to exit
BrowserFullscreen-->>SliceHeaderControls: Fire fullscreen change event
SliceHeaderControls->>ChartHolder: Sync fullsize state to normal mode
```
---
*Generated by [CodeAnt AI](https://codeant.ai)*
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "venkateshwaracholan (via GitHub)" <gi...@apache.org>.
venkateshwaracholan commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2981137066
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
+ &&[data-test='dashboard-component-chart-holder']:fullscreen {
Review Comment:
done
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4118085862
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: implement native browser fullscreen for dashboard charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4218751926
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: implement native browser fullscreen for dashboard charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4217851129
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: implement native browser fullscreen for dashboard charts [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4192410009
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/38819?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `65.45455%` with `19 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.52%. Comparing base ([`d796543`](https://app.codecov.io/gh/apache/superset/commit/d796543f5a0c5f6306f3140d48647640b7a28a14?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a07748a`](https://app.codecov.io/gh/apache/superset/commit/a07748a11541fcbce3c3fdd440f780441ffcfc14?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/38819?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...dashboard/components/SliceHeaderControls/index.tsx](https://app.codecov.io/gh/apache/superset/pull/38819?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fdashboard%2Fcomponents%2FSliceHeaderControls%2Findex.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyQ29udHJvbHMvaW5kZXgudHN4) | 51.72% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/38819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ponents/gridComponents/ChartHolder/ChartHolder.tsx](https://app.codecov.io/gh/apache/superset/pull/38819?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fdashboard%2Fcomponents%2FgridComponents%2FChartHolder%2FChartHolder.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0SG9sZGVyL0NoYXJ0SG9sZGVyLnRzeA==) | 79.16% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/38819?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #38819 +/- ##
==========================================
- Coverage 64.52% 64.52% -0.01%
==========================================
Files 2536 2537 +1
Lines 131208 131250 +42
Branches 30457 30473 +16
==========================================
+ Hits 84661 84686 +25
- Misses 45084 45101 +17
Partials 1463 1463
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/38819/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/38819/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.06% <65.45%> (-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=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/38819?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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "venkateshwaracholan (via GitHub)" <gi...@apache.org>.
venkateshwaracholan commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4118129069
@msyavuz I have addressed the review comments and made the changes, Please review.
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4162328301
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "venkateshwaracholan (via GitHub)" <gi...@apache.org>.
venkateshwaracholan commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2981377671
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
+ [data-test='dashboard-component-chart-holder']:fullscreen {
+ background-color: ${theme.colorBgBase};
+ width: 100vw;
+ height: 100vh;
+ display: flex;
+ flex-direction: column;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: visible;
+ position: relative;
+ pointer-events: auto;
+ z-index: 1000;
Review Comment:
Swapped the magic number for ${theme.zIndexPopupBase} to use the standardized theme token
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca merged PR #38819:
URL: https://github.com/apache/superset/pull/38819
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r3061209643
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -29,7 +29,8 @@ import { Refs } from '../types';
export function getDefaultTooltip(refs: Refs) {
return {
- appendToBody: true,
+ appendToBody:
+ typeof document !== 'undefined' ? !document.fullscreenElement : true,
Review Comment:
Works today, but through a chain that's easy to miss: fullscreen flips `chartWidth`/`chartHeight` in `ChartHolder`, `transformProps` re-runs, `getDefaultTooltip` gets re-evaluated (now with `document.fullscreenElement` set), the new `echartOptions` trips `Echart.tsx`'s setOption effect, and the `notMerge: true` path re-inits the tooltip with the fresh `appendToBody`. Spot-checked in the browser — tooltip parent during fullscreen is the chart container div, not body.
You're right that it's fragile though. Three layers of implicit coupling and any of them changing silently breaks the fix. If you'd rather lock it down, I can add a `fullscreenchange` listener in `Echart.tsx` as a follow-up that does `setOption({ tooltip: { appendToBody: !document.fullscreenElement } }, { replaceMerge: ['tooltip'] })`. WDYT?
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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
Review Comment:
Already fixed — @michael-s-molina merged your suggestion as 0c6ace32bd.
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "venkateshwaracholan (via GitHub)" <gi...@apache.org>.
venkateshwaracholan commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2981374409
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -94,8 +97,10 @@ const VerticalDotsTrigger = () => {
/>
);
};
+const FULLSCREEN_Z_INDEX = 10001;
Review Comment:
Replaced the hardcoded constant with supersetTheme.zIndexPopupBase + 1 to align with the AntD design tokens.
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4161973414
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 0c6ace32bd52c7fb5e9203fd45f25d2dbcab213f |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69cbafc0f22cc7000951dbdb |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-38819--superset-docs-preview.netlify.app](https://deploy-preview-38819--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-38819--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4171733148
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: implement native browser fullscreen for dashboard charts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r3039694897
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -231,9 +235,34 @@ const SliceHeaderControls = (
// eslint-disable-next-line no-unused-expressions
props.exportPivotCSV?.(props.slice.slice_id);
break;
- case MenuKeys.Fullscreen:
- props.handleToggleFullSize();
+ case MenuKeys.Fullscreen: {
+ const el = props.chartHolderRef?.current;
+
+ if (!document.fullscreenElement) {
+ el?.requestFullscreen?.()
+ .then(() => {
+ props.handleToggleFullSize();
+ // Trigger resize to fit the fullscreen container
+ setTimeout(() => {
+ window.dispatchEvent(new Event('resize'));
+ }, 300);
+ })
Review Comment:
Can this cause a race condition with the
```ts
const handleFullscreenChange = () => {
if (!document.fullscreenElement && isFullSize) {
props.handleToggleFullSize();
}
};
```
below? When you open it the one below doesn't run but when you close it both runs right?
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "venkateshwaracholan (via GitHub)" <gi...@apache.org>.
venkateshwaracholan commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4248520067
> [richardfogaca](/richardfogaca)
thanks @msyavuz and @JCelento for the reviews and guidance along the way.
@richardfogaca
Thanks a lot for stepping in and helping push this over the line, I really appreciate the time and care you put into the follow-ups.
The changes around fullscreenchange, tooltip handling and resize scoping were especially helpful to go through. There’s a lot I can learn from how you handled those edge cases, I’ll spend some time digging into them properly.
Really appreciate the learning experience here, and glad to have my first PR in Superset merged.
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4251547098
Congratulations on your first superset PR @venkateshwaracholan, exciting improvement! :)
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "venkateshwaracholan (via GitHub)" <gi...@apache.org>.
venkateshwaracholan commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2981328645
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
Review Comment:
yes, removed it
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
Review Comment:
yes, removed it now, thanks
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "venkateshwaracholan (via GitHub)" <gi...@apache.org>.
venkateshwaracholan commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2981416480
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ [data-test='dashboard-component-chart-holder']:fullscreen {
+ background-color: ${theme.colorBgBase};
+ width: 100vw;
+ height: 100vh;
+ display: flex;
Review Comment:
Fixed. Added box-sizing: border-box to include padding within the viewport and prevent overflow.
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4118574052
**Bito Review Failed - Technical Failure** Bito encountered technical difficulties while starting a code review session. To retry, type `/review` in a comment and save. If the issue persists, contact support@bito.ai.
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r2979514254
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ /* Using && to increase specificity without needing !important as often */
+ &&[data-test='dashboard-component-chart-holder']:fullscreen {
Review Comment:
**Suggestion:** The selector starts with `&&` inside a `Global` style block, which has no parent selector to expand `&`. That makes the fullscreen rule invalid and causes the main fullscreen layout styles to be skipped. Use a normal explicit selector so the rule actually matches the fullscreen chart holder. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ⚠️ Fullscreen chart may not get intended 100vw/100vh layout.
- ⚠️ New fullscreen feature can appear partially broken.
```
</details>
```suggestion
[data-test='dashboard-component-chart-holder']:fullscreen {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard chart menu and click fullscreen; `MenuKeys.Fullscreen` in
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:239-249` calls
`el.requestFullscreen()` then `props.handleToggleFullSize()`.
2. `handleToggleFullSize` in
`superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx:232-234`
sets fullsize state, making `isFullSize` true for that chart.
3. With `isFullSize === true`, `SliceHeaderControls` renders `<Global
styles={fullscreenStyles(theme)} />` at
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:656`.
4. In `superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:23`, the
primary fullscreen selector starts with `&&...`; in a global stylesheet there is no parent
selector context, so this selector does not reliably match, causing the main fullscreen
sizing/layout declarations in that block to be skipped.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx
**Line:** 23:23
**Comment:**
*Logic Error: The selector starts with `&&` inside a `Global` style block, which has no parent selector to expand `&`. That makes the fullscreen rule invalid and causes the main fullscreen layout styles to be skipped. Use a normal explicit selector so the rule actually matches the fullscreen chart holder.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38819&comment_hash=96bff3cc9427817978908929ab34c11adc7a48f893243043a31e592b0f6e233c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38819&comment_hash=96bff3cc9427817978908929ab34c11adc7a48f893243043a31e592b0f6e233c&reaction=dislike'>👎</a>
--
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] feat: implement native browser fullscreen for dashboard charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38819:
URL: https://github.com/apache/superset/pull/38819#issuecomment-4115908435
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "JCelento (via GitHub)" <gi...@apache.org>.
JCelento commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r3047747319
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -554,13 +583,32 @@ const SliceHeaderControls = (
});
}
+ useEffect(() => {
+ const handleFullscreenChange = () => {
+ if (!document.fullscreenElement && isFullSize) {
+ props.handleToggleFullSize();
+ }
+ };
+
+ document.addEventListener('fullscreenchange', handleFullscreenChange);
+ return () => {
+ document.removeEventListener('fullscreenchange', handleFullscreenChange);
+ };
+ }, [isFullSize, props]);
Review Comment:
props is a new object every render, so this effect re-subscribes on every render and can briefly miss `fullscreenchange`. Can we change this to `[isFullSize, props.handleToggleFullSize]` (or the minimal stable callbacks actually used)?
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -29,7 +29,8 @@ import { Refs } from '../types';
export function getDefaultTooltip(refs: Refs) {
return {
- appendToBody: true,
+ appendToBody:
+ typeof document !== 'undefined' ? !document.fullscreenElement : true,
Review Comment:
`getDefaultTooltip` runs when options are built; if that does not re-run when entering
fullscreen, `appendToBody` can stay wrong, and tooltips may still target document.body outside the
fullscreen element. WDYT about updating tooltip options on fullscreenchange / via setOption on the
chart instance so it stays in sync.
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r3060608042
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -554,13 +583,32 @@ const SliceHeaderControls = (
});
}
+ useEffect(() => {
+ const handleFullscreenChange = () => {
+ if (!document.fullscreenElement && isFullSize) {
+ props.handleToggleFullSize();
+ }
+ };
+
+ document.addEventListener('fullscreenchange', handleFullscreenChange);
+ return () => {
+ document.removeEventListener('fullscreenchange', handleFullscreenChange);
+ };
+ }, [isFullSize, props]);
Review Comment:
Fixed in commit b724f77b57. The effect now depends on the minimal stable values it actually uses instead of the full `props` object, so it does not resubscribe on every render.
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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 { css, SupersetTheme } from '@apache-superset/core/theme';
+
+export const fullscreenStyles = (theme: SupersetTheme) => css`
+ [data-test='dashboard-component-chart-holder']:fullscreen {
+ background-color: ${theme.colorBgBase};
+ width: 100vw;
+ height: 100vh;
+ box-sizing: border-box;
+ display: flex;
+ flex-direction: column;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: visible;
+ position: relative;
+ pointer-events: auto;
+ z-index: ${theme.zIndexPopupBase};
+ opacity: 1;
+ visibility: visible;
+
+ /* Ensure children take up available space */
+ .dashboard-chart,
+ .chart-container,
+ .slice_container,
+ .chart-slice {
+ flex: 1 1 auto;
+ height: 100%;
+ width: 100%;
+ display: flex;
+ flex-direction: column;
+ overflow: visible;
+ }
+
+ /* Portaled components inside the fullscreen layer */
+ .ant-dropdown,
+ .ant-tooltip,
+ .ant-modal-root,
+ .ant-select-dropdown,
+ .ant-popover {
+ z-index: ${theme.zIndexPopupBase + 1};
+ pointer-events: auto;
+ }
+ }
+
+ /* Interaction and Header fixes */
+ [data-test='dashboard-component-chart-holder']:fullscreen * {
+ pointer-events: auto;
+ }
+
+ [data-test='dashboard-component-chart-header'] {
Review Comment:
Fixed in commit b724f77b57. The fullscreen z-index rule now targets the real `[data-test="slice-header"]` element under `[data-test="dashboard-component-chart-holder"]:fullscreen`, which matches the DOM and restores the header layering fix.
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -231,9 +235,34 @@ const SliceHeaderControls = (
// eslint-disable-next-line no-unused-expressions
props.exportPivotCSV?.(props.slice.slice_id);
break;
- case MenuKeys.Fullscreen:
- props.handleToggleFullSize();
+ case MenuKeys.Fullscreen: {
+ const el = props.chartHolderRef?.current;
+
+ if (!document.fullscreenElement) {
+ el?.requestFullscreen?.()
+ .then(() => {
+ props.handleToggleFullSize();
+ // Trigger resize to fit the fullscreen container
+ setTimeout(() => {
+ window.dispatchEvent(new Event('resize'));
+ }, 300);
+ })
Review Comment:
Fixed in commit b724f77b57. `fullscreenchange` is now the single sync point for native fullscreen state, and the enter/exit handlers only request or exit browser fullscreen (with a local fallback when the API is unavailable), so we no longer double-toggle on exit.
--
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: implement native browser fullscreen for dashboard charts [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #38819:
URL: https://github.com/apache/superset/pull/38819#discussion_r3009866407
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:
##########
@@ -0,0 +1,70 @@
+/**
+ * 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
Review Comment:
Not sure if it matters but there is a backtick here, probably an artifact
```suggestion
* with the License. You may obtain a copy of the License at
```
--
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