You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "LuisSanchez (via GitHub)" <gi...@apache.org> on 2026/01/09 04:12:18 UTC
[PR] chore(charts): echars left padding too big and automation of title [superset]
LuisSanchez opened a new pull request, #36993:
URL: https://github.com/apache/superset/pull/36993
### SUMMARY
#### Problem
Charts had too much left margin by default.
#### Solution
- Add 0 margin as an option (and default) on the X and Y axis
- Automatically set margin to 30 when an axis title is added and margin is 0
- Automatically reset margin to 0 when an axis title is removed
- Apply padding offsets only when both title exists and margin is non-zero
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
#### BEFORE
<img width="1692" height="820" alt="image" src="https://github.com/user-attachments/assets/10ed4786-441f-4cc4-b046-ae00b250ae89" />
#### AFTER
https://github.com/user-attachments/assets/b6b7bb4c-8254-4424-87fb-9356ff77ebc1
### TESTING INSTRUCTIONS
1. Add a new chart.
2. Check the left margin that is too big, instead of 0.
3. Add a title on the Y axis and see how it grows automatically.
4. Add a title on the X axis and see how it grows automatically.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: 96451
- [ ] 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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718799297
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
Indeed, test updated to simulate control changes by dispatching Redux actions and verify that the automatic margin adjustment logic works correctly
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3825110717
⚠️ **DEPRECATED WORKFLOW** ⚠️
@EnxDev This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "🎪 trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/21526606772). 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] chore(charts): echars left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3727118651
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3787213483
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3820971768
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21498807363) for [fbe751c](https://github.com/apache/superset/commit/fbe751c)
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2742426383
##########
superset-frontend/packages/superset-ui-chart-controls/src/sections/chartTitle.tsx:
##########
@@ -23,7 +23,7 @@ import { ControlPanelSectionConfig } from '../types';
import { formatSelectOptions } from '../utils';
export const TITLE_MARGIN_OPTIONS: number[] = [
- 15, 30, 50, 75, 100, 125, 150, 200,
+ 0, 15, 30, 50, 75, 100, 125, 150, 200,
Review Comment:
Yes, it is intended and it was verify.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3825149136
@EnxDev Ephemeral environment spinning up at http://34.210.70.51: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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3772823621
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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] chore(charts): echarts left padding too big and automation of title [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 #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2692705496
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -19,10 +19,14 @@
import { CategoricalColorScale } from '@superset-ui/core';
import type { SeriesOption } from 'echarts';
import { EchartsTimeseriesSeriesType } from '../../src';
+import { TIMESERIES_CONSTANTS } from '../../src/constants';
+import { LegendOrientation } from '../../src/types';
import {
transformSeries,
transformNegativeLabelsPosition,
+ getPadding,
} from '../../src/Timeseries/transformers';
Review Comment:
**Suggestion:** The test imports `getPadding` from '../../src/Timeseries/transformers' but that function is not exported from that module (the real helper that is being spied on is `getChartPadding` in '../../src/utils/series'). Importing the wrong symbol will cause the test module to fail to load or call an undefined function; replace the import so tests call the actual util exported from the utils module. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Jest tests fail loading this test file.
- ❌ CI job for chart-echarts tests will error.
- ⚠️ Blocks PR validation for related chart plugin.
```
</details>
```suggestion
} from '../../src/Timeseries/transformers';
import { getChartPadding as getPadding } from '../../src/utils/series';
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test file imports at
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts lines
22-29 where getPadding is imported from ../../src/Timeseries/transformers (see line 27 in
file). The test expects a function named getPadding to be present.
2. Jest starts loading the test module and resolves imports. Jest will attempt to import
getPadding from ../../src/Timeseries/transformers at module load time (import occurs at
file top, lines 22-29).
3. The actual transformers module (src/Timeseries/transformers.ts) in the final code
exports transformSeries and transformNegativeLabelsPosition but contains no exported
getPadding function (inspect transformers.ts: transformSeries is defined starting around
line 167; no getPadding export exists).
4. Because getPadding is not exported from transformers, the test module will either
receive undefined for getPadding or the import will fail to resolve the symbol, causing
the test file to error when it calls getPadding (call sites are at
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:229
where getPadding(...) is invoked). This triggers an immediate Jest load-time failure for
this test file.
5. Replace the import to pull getChartPadding from src/utils/series (the test already
spies on seriesUtils.getChartPadding), or re-export a proper getPadding wrapper from
transformers if intentional. The improved import maps getChartPadding to getPadding so
call sites continue to work with the spy.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
**Line:** 27:28
**Comment:**
*Possible Bug: The test imports `getPadding` from '../../src/Timeseries/transformers' but that function is not exported from that module (the real helper that is being spied on is `getChartPadding` in '../../src/utils/series'). Importing the wrong symbol will cause the test module to fail to load or call an undefined function; replace the import so tests call the actual util exported from the utils module.
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>
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3785931563
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21259512729) for [10f9b21](https://github.com/apache/superset/commit/10f9b21)
--
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] chore(charts): echars left padding too big and automation of title [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3727126201
## Nitpicks 🔍
<table>
<tr><td>🔒 <strong>No security issues identified</strong></td></tr>
<tr><td>⚡ <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-26ee6b02af1970f54d4dfaffabd065e966b064b85132577816b08c8ddaabeb6fR26-R85'><strong>UX / backward-compatibility</strong></a><br>Changing the default margin from a non-zero value to 0 changes the initial layout for existing and new charts. Confirm this is intentional for all affected chart types and that tests or snapshots are updated. Also confirm automation that sets margin to 30 when a title is added still triggers reliably when starting from 0.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-88405b689ea6064a26842b88ad09e219f3f2e22d9bd031303c4d7b2a518e68f0R562-R583'><strong>Numeric comparison risk</strong></a><br>The new code compares margin values using strict equality against 0, but margin control values may be strings (coming from form controls). This can cause the branch that sets margins to 30 or 0 to be skipped unexpectedly. Convert or coerce margin values to numbers before comparisons.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-d73c2cf4fc7d8cbaf9d0aee4ba2ecc465568d353e29f70dc244af1a4f464a647R245-R256'><strong>Missing automatic margin update</strong></a><br>The PR description promises automatically setting margin to 30 when an axis title is added (and resetting to 0 when removed). The new lines only compute whether to add title offsets to padding but do not compute or apply an "effective" margin value (or mutate formData). Verify where/how the automatic margin change is implemented — if it's not, consider computing an effective margin here (without mutating formData) or implementing the automation at the form layer.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-2672bc046f1a56f67dd661b1ace7c639da51ad78c4e442b96445c60924621c4eR553-R557'><strong>Inconsistent effective margin handling</strong></a><br>The new booleans `addYAxisTitleOffset` / `addXAxisTitleOffset` gate adding extra padding based on whether the configured title margin is non-zero. However, axis `nameGap` and the `chartPadding` call still use the raw converted margins (via `convertInteger(...)`) rather than an "effective" margin that could be auto-set (e.g., to 30) when a title is added and the margin is 0. Ensure the code that "automatically set[s] margin to 30 when an axis title is added and margin is 0" is applied consistently to both determining padding and the axis `nameGap`.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36993/files#diff-5f5c22612f270fe5718a3d200d291ec22bc1ee0bd576ef16f36e76d286cddb44R646-R661'><strong>Offset applied when title missing</strong></a><br>The new ternary still applies `yAxisOffset` in the fallback branch when `yAxisTitlePosition` is falsy. Per the PR description, offsets should be applied only when an axis title exists and margin is non-zero — verify whether the code should avoid adding `yAxisOffset` when `yAxisTitlePosition` is undefined/empty.<br>
</td></tr>
</table>
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2741369758
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -571,6 +571,29 @@ function ExploreViewContainer(props) {
}
}
+ // Automatically set axis title margins when titles are added or removed
+ if (changedControlKeys.includes('x_axis_title')) {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = props.controls.x_axis_title_margin?.value ?? 0;
+
+ if (xAxisTitle && currentMargin < 30) {
+ props.actions.setControlValue('x_axis_title_margin', 30);
+ } else if (!xAxisTitle && currentMargin !== 0) {
+ props.actions.setControlValue('x_axis_title_margin', 0);
+ }
+ }
+
+ if (changedControlKeys.includes('y_axis_title')) {
+ const yAxisTitle = props.controls.y_axis_title?.value || '';
+ const currentMargin = props.controls.y_axis_title_margin?.value ?? 0;
+
Review Comment:
Logic was updated to automatically set margin to 30 when adding / updating title if current margin is less than 30 (X and Y axis).
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2707605137
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
Bot seems correct on these comments. Tests don't seem to check what they say they check
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718799383
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718797902
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
Indeed, test updated to simulate control changes.
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
Indeed, test updated to simulate control changes.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718800748
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<!-- Bito Reply -->
The test update simulates control changes by dispatching an action to add an X axis title and verifying that the margin is automatically set to 30.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3740369145
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3752542419
CodeAnt AI Incremental review completed.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2708583185
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -576,8 +576,11 @@ export default function transformProps(
? getXAxisFormatter(xAxisTimeFormat)
: String;
- const addYAxisTitleOffset = !!(yAxisTitle || yAxisTitleSecondary);
- const addXAxisTitleOffset = !!xAxisTitle;
+ const addYAxisTitleOffset =
+ !!(yAxisTitle || yAxisTitleSecondary) &&
+ convertInteger(yAxisTitleMargin) !== 0;
+ const addXAxisTitleOffset =
+ !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Potential title overlap on zero margin</b></div>
<div id="fix">
The update conditions title offset addition on margin !== 0, potentially causing axis titles to overlap the chart area when margins are set to 0. Previously, offsets were added whenever titles existed, ensuring space. This matches Timeseries and BoxPlot logic but changes behavior for margin=0 cases.
</div>
</div>
<small><i>Code Review Run #ab83e6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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] chore(charts): echars left padding too big and automation of title [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3727124044
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 027d68d13622d34cc7f80efecd18108b3c5a676e |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/696080225263b70008b1ff66 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-36993--superset-docs-preview.netlify.app](https://deploy-preview-36993--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-36993--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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718803375
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change Y axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change Y axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment handles both X and Y axis titles being set simultaneously', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle both axes independently
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<!-- Bito Reply -->
The test file has been updated with new test cases for the getPadding function, simulating control changes for Y-axis title positions ('Left' and 'Top') to verify correct margin application in chart padding.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718797165
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
Indeed, test updated to simulate control changes.
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
Indeed, test updated to simulate control changes.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3785931557
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21259512786) for [10f9b21](https://github.com/apache/superset/commit/10f9b21)
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3740574396
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] chore(charts): echars left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2680531668
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -559,6 +559,29 @@ function ExploreViewContainer(props) {
}
}
+ // Automatically set axis title margins when titles are added or removed
+ if (changedControlKeys.includes('x_axis_title')) {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = props.controls.x_axis_title_margin?.value ?? 0;
Review Comment:
Yes, if we skip this change, we will avoid setting automatically the margins to a default of 30 or 0 if the title changes.
If we don't add this, we leave the update of the margins to the final user, giving, imho, a bad experience
For example, it will look like this instead of the "after" in the description
https://github.com/user-attachments/assets/64f161b8-1500-4274-bc7d-43f4847e37fd
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3817308996
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2741664771
##########
superset-frontend/packages/superset-ui-chart-controls/src/sections/chartTitle.tsx:
##########
@@ -23,7 +23,7 @@ import { ControlPanelSectionConfig } from '../types';
import { formatSelectOptions } from '../utils';
export const TITLE_MARGIN_OPTIONS: number[] = [
- 15, 30, 50, 75, 100, 125, 150, 200,
+ 0, 15, 30, 50, 75, 100, 125, 150, 200,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Shared constant index shift affects defaults</b></div>
<div id="fix">
Adding 0 to the start of TITLE_MARGIN_OPTIONS shifts all indices, changing default margins for Bubble chart (y-axis from 30 to 15) and Bar chart (x-axis from 15 to 0, y-axis from 30 to 15). Since this shared constant is imported by other plugins, verify if these behavioral changes are intended or update the affected defaults.
</div>
</div>
<small><i>Code Review Run #161e41</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3837870115
Looks like QA passed so removing the testing label and merging.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3753582322
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/21024619970) for [c3e3593](https://github.com/apache/superset/commit/c3e3593)
• **Environment:** http://16.145.14.40:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3785970706
@sadpandajoe Ephemeral environment spinning up at http://52.40.152.72: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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3786026489
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/21259512786) for [10f9b21](https://github.com/apache/superset/commit/10f9b21)
• **Environment:** http://34.220.56.65:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3817657446
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3771983401
This also needs another rebase
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3824046103
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21519084718) for [fbe751c](https://github.com/apache/superset/commit/fbe751c)
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718801651
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<!-- Bito Reply -->
The added tests in ExploreViewContainer.test.tsx simulate different Redux state configurations for axis title controls (e.g., title presence and margin values) by setting custom initialState in renderWithRouter. They verify the component renders without errors, confirming the automatic margin adjustment logic (setting margin to 30 when title is added and margin is 0, resetting to 0 when title is removed, and preserving non-zero margins) works as expected.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3752563052
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2684003565
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change Y axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Y axis non-zero margin test inadequate</b></div>
<div id="fix">
Similar for Y axis.
</div>
</div>
<small><i>Code Review Run #123592</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Non-zero margin test inadequate</b></div>
<div id="fix">
Test sets empty title initially, doesn't simulate adding title.
</div>
</div>
<small><i>Code Review Run #123592</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inadequate test coverage for margin adjustment</b></div>
<div id="fix">
The test claims to verify automatic X axis margin adjustment when title is added, but it sets an empty title and doesn't simulate a control change, so the adjustment logic isn't exercised. This means bugs in the margin logic could go undetected.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/d858cbc/AGENTS.md#L22">AGENTS.md:22</a>
</li>
</ul>
</details>
<small><i>Code Review Run #123592</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inadequate test coverage for Y axis margin</b></div>
<div id="fix">
Similar to the X axis test, this doesn't simulate a control change to add a title, so the adjustment isn't tested.
</div>
</div>
<small><i>Code Review Run #123592</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Reset test doesn't simulate title removal</b></div>
<div id="fix">
Test sets a title but doesn't simulate removal, so reset logic isn't tested.
</div>
</div>
<small><i>Code Review Run #123592</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Y axis reset test inadequate</b></div>
<div id="fix">
Similar issue for Y axis reset.
</div>
</div>
<small><i>Code Review Run #123592</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change Y axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment handles both X and Y axis titles being set simultaneously', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle both axes independently
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Simultaneous titles test inadequate</b></div>
<div id="fix">
Test sets empty titles, doesn't simulate setting them.
</div>
</div>
<small><i>Code Review Run #123592</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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] chore(charts): echars left padding too big and automation of title [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3727127364
CodeAnt AI finished reviewing your 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] chore(charts): echars left padding too big and automation of title [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 #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2674760341
##########
superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts:
##########
@@ -242,8 +242,10 @@ export default function transformProps(
// @ts-ignore
...outlierData,
];
- const addYAxisTitleOffset = !!yAxisTitle;
- const addXAxisTitleOffset = !!xAxisTitle;
+ const addYAxisTitleOffset =
+ !!yAxisTitle && convertInteger(yAxisTitleMargin) !== 0;
+ const addXAxisTitleOffset =
+ !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
Review Comment:
**Suggestion:** `convertInteger` is invoked multiple times (here and later when building padding), which is redundant and can cause inconsistent results if the conversion is non-deterministic; call `convertInteger` once per margin and reuse the parsed integer values. [performance]
**Severity Level:** Minor ⚠️
```suggestion
const yAxisTitleMarginInt = convertInteger(yAxisTitleMargin);
const xAxisTitleMarginInt = convertInteger(xAxisTitleMargin);
const addYAxisTitleOffset =
!!yAxisTitle && yAxisTitleMarginInt !== 0;
const addXAxisTitleOffset =
!!xAxisTitle && xAxisTitleMarginInt !== 0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid, practical improvement. convertInteger is called again later when building chartPadding; parsing once into local yAxisTitleMarginInt/xAxisTitleMarginInt and reusing those values avoids duplicate work and ensures consistent behavior if convertInteger ever changes. It's low risk and improves clarity.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts
**Line:** 245:248
**Comment:**
*Performance: `convertInteger` is invoked multiple times (here and later when building padding), which is redundant and can cause inconsistent results if the conversion is non-deterministic; call `convertInteger` once per margin and reuse the parsed integer values.
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>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -550,8 +550,11 @@ export default function transformProps(
? getXAxisFormatter(xAxisTimeFormat)
: String;
- const addYAxisTitleOffset = !!(yAxisTitle || yAxisTitleSecondary);
- const addXAxisTitleOffset = !!xAxisTitle;
+ const addYAxisTitleOffset =
+ !!(yAxisTitle || yAxisTitleSecondary) &&
+ convertInteger(yAxisTitleMargin) !== 0;
+ const addXAxisTitleOffset =
+ !!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
Review Comment:
**Suggestion:** `convertInteger` is called multiple times for the same margin values (here and later); compute the integer margins once and reuse them to avoid repeated parsing and improve clarity. [performance]
**Severity Level:** Minor ⚠️
```suggestion
const yAxisTitleMarginInt = convertInteger(yAxisTitleMargin ?? 0);
const xAxisTitleMarginInt = convertInteger(xAxisTitleMargin ?? 0);
const addYAxisTitleOffset =
!!(yAxisTitle || yAxisTitleSecondary) && yAxisTitleMarginInt !== 0;
const addXAxisTitleOffset = !!xAxisTitle && xAxisTitleMarginInt !== 0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Good suggestion — convertInteger is called multiple times for the same inputs later
(chartPadding and axis nameGap). Converting once and reusing the integer improves
clarity and avoids repeated parseInt calls. It's a small, safe optimization with real benefit.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 553:557
**Comment:**
*Performance: `convertInteger` is called multiple times for the same margin values (here and later); compute the integer margins once and reuse them to avoid repeated parsing and improve clarity.
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>
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -559,6 +559,29 @@ function ExploreViewContainer(props) {
}
}
+ // Automatically set axis title margins when titles are added or removed
+ if (changedControlKeys.includes('x_axis_title')) {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = props.controls.x_axis_title_margin?.value ?? 0;
Review Comment:
**Suggestion:** Type mismatch in numeric comparison: `currentMargin` may be a string (e.g. "0" or "30") or another non-numeric value, so strict comparisons `currentMargin === 0` and `currentMargin !== 0` can behave incorrectly. Convert the control value to a number (with a safe fallback to 0) before comparing. [type error]
**Severity Level:** Minor ⚠️
```suggestion
const xAxisTitleRaw = props.controls.x_axis_title?.value || '';
const xAxisTitle =
typeof xAxisTitleRaw === 'string' ? xAxisTitleRaw : String(xAxisTitleRaw);
const currentMarginRaw = props.controls.x_axis_title_margin?.value;
const currentMargin = Number.isFinite(Number(currentMarginRaw))
? Number(currentMarginRaw)
: 0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing comparisons (=== 0 / !== 0) can misbehave if the control stores margin values as strings (e.g. "0" or "30"). Coercing/normalizing to Number before comparing is a legitimate fix for a real logic bug that could leave margins stuck in the wrong state.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
**Line:** 564:565
**Comment:**
*Type Error: Type mismatch in numeric comparison: `currentMargin` may be a string (e.g. "0" or "30") or another non-numeric value, so strict comparisons `currentMargin === 0` and `currentMargin !== 0` can behave incorrectly. Convert the control value to a number (with a safe fallback to 0) before comparing.
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>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -656,7 +656,9 @@ export function getPadding(
top:
yAxisTitlePosition && yAxisTitlePosition === 'Top'
? TIMESERIES_CONSTANTS.gridOffsetTop + (Number(yAxisTitleMargin) || 0)
- : TIMESERIES_CONSTANTS.gridOffsetTop + yAxisOffset,
+ : yAxisTitlePosition === 'Left'
+ ? TIMESERIES_CONSTANTS.gridOffsetTop
Review Comment:
**Suggestion:** Logic error: when the Y-axis title is positioned on the left the top padding branch returns just `TIMESERIES_CONSTANTS.gridOffsetTop` and ignores `yAxisOffset`, which means the Y-axis label top offset (when `addYAxisTitleOffset` is true) will not be applied and can cause overlap of the chart content with the axis title; include `yAxisOffset` in the 'Left' branch so the top padding accounts for the offset. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
? TIMESERIES_CONSTANTS.gridOffsetTop + yAxisOffset
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The left-branch currently returns TIMESERIES_CONSTANTS.gridOffsetTop without adding yAxisOffset,
so when addYAxisTitleOffset is true the top padding will be too small for the Left case.
The improved code restores consistent behavior with the other branch (including the yAxisOffset)
and fixes a real layout bug. This is a localized logic fix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
**Line:** 660:660
**Comment:**
*Logic Error: Logic error: when the Y-axis title is positioned on the left the top padding branch returns just `TIMESERIES_CONSTANTS.gridOffsetTop` and ignores `yAxisOffset`, which means the Y-axis label top offset (when `addYAxisTitleOffset` is true) will not be applied and can cause overlap of the chart content with the axis title; include `yAxisOffset` in the 'Left' branch so the top padding accounts for the offset.
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>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -190,3 +194,149 @@ describe('transformNegativeLabelsPosition', () => {
expect((result as any)[4].label).toBe(undefined);
});
});
+
+describe('getPadding', () => {
+ let getChartPaddingSpy: jest.SpyInstance;
+
+ beforeEach(() => {
+ // Mock getChartPadding to return the padding object as-is for easier testing
+ getChartPaddingSpy = jest.spyOn(seriesUtils, 'getChartPadding');
+ getChartPaddingSpy.mockImplementation(
+ (
+ show: boolean,
+ orientation: LegendOrientation,
+ margin: string | number | null | undefined,
+ padding:
+ | {
+ bottom?: number;
+ left?: number;
+ right?: number;
+ top?: number;
+ }
+ | undefined,
+ ) => {
+ return {
+ bottom: padding?.bottom ?? 0,
+ left: padding?.left ?? 0,
+ right: padding?.right ?? 0,
+ top: padding?.top ?? 0,
+ };
+ },
+ );
+ });
+
+ afterEach(() => {
+ getChartPaddingSpy.mockRestore();
Review Comment:
**Suggestion:** `afterEach` calls `getChartPaddingSpy.mockRestore()` unguarded; if the spy wasn't created (e.g., beforeEach failed), calling `mockRestore` will throw. Guard the restore call to run only when the spy is defined to make the teardown robust. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
if (getChartPaddingSpy) {
getChartPaddingSpy.mockRestore();
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a pragmatic, defensive fix for the test teardown. If beforeEach throws and the spy isn't assigned,
calling mockRestore on undefined will throw and obscure the original failure. Guarding the restore makes
the teardown robust and avoids noisy errors in CI. This addresses a real risk in test execution, not just a
stylistic preference.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
**Line:** 229:229
**Comment:**
*Possible Bug: `afterEach` calls `getChartPaddingSpy.mockRestore()` unguarded; if the spy wasn't created (e.g., beforeEach failed), calling `mockRestore` will throw. Guard the restore call to run only when the spy is defined to make the teardown robust.
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>
--
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] chore(charts): echars left padding too big and automation of title [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3727118549
CodeAnt AI is reviewing your PR.
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3817308360
Latest commit: automatically set margin to 30 when adding / updating title if current margin is less than 30 (X and Y axis). Added tests for this.
cc @msyavuz
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3752690900
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3752533769
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3753523531
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21024619970) for [c3e3593](https://github.com/apache/superset/commit/c3e3593)
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3773096126
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2707616166
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<!-- Bito Reply -->
You're right—the tests added for the automatic axis title margin adjustment feature only verify that no error tooltips appear, but they don't actually check if the margins are set to 30 when titles are added or reset to 0 when removed. The test names suggest they validate the margin logic, but the assertions are too shallow.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718800122
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change Y axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
Indeed, test updated to simulate control changes.
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets X axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment resets Y axis margin to 0 when title is removed', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: 'Y Axis Label' },
+ y_axis_title_margin: { value: 30 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle title removal and reset margin
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change X axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment does not change Y axis margin when title is added but margin is already non-zero', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 50 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should not override user-set margin values
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment handles both X and Y axis titles being set simultaneously', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Component should handle both axes independently
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
Indeed, test updated to simulate control changes.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2683815397
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,181 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+describe('Automatic axis title margin adjustment', () => {
Review Comment:
Done by following avoid nesting when testing principle!
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -190,3 +194,149 @@ describe('transformNegativeLabelsPosition', () => {
expect((result as any)[4].label).toBe(undefined);
});
});
+
+describe('getPadding', () => {
Review Comment:
Done by following avoid nesting when testing principle!
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3772826751
CodeAnt AI Incremental review completed.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2742429688
##########
superset-frontend/packages/superset-ui-chart-controls/src/sections/chartTitle.tsx:
##########
@@ -23,7 +23,7 @@ import { ControlPanelSectionConfig } from '../types';
import { formatSelectOptions } from '../utils';
export const TITLE_MARGIN_OPTIONS: number[] = [
- 15, 30, 50, 75, 100, 125, 150, 200,
+ 0, 15, 30, 50, 75, 100, 125, 150, 200,
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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] chore(charts): echarts left padding too big and automation of title [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 #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2741366500
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -136,7 +142,7 @@ const renderWithRouter = ({
<ExploreViewContainer />
</Route>
</MemoryRouter>,
- { useRedux: true, useDnd: true, initialState },
+ { useRedux: true, useDnd: true, initialState, store },
);
Review Comment:
**Suggestion:** `renderWithRouter` overwrites `window.location` without preserving or restoring the original value, which can leak state between tests and break other browser APIs; save the original `window.location` and restore it after calling `render`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ExploreViewContainer tests may become flaky due to leaked location.
- ⚠️ Tests asserting window.history.replaceState behave unpredictably.
- ⚠️ Cross-test interference inside the same test file.
```
</details>
```suggestion
const originalLocation = window.location;
Object.defineProperty(window, 'location', {
get() {
return { pathname: path, search };
},
configurable: true,
});
try {
return render(
<MemoryRouter initialEntries={[`${path}${search}`]}>
<Route path={path}>
<ExploreViewContainer />
</Route>
</MemoryRouter>,
{ useRedux: true, useDnd: true, initialState, store },
);
} finally {
// restore original location to avoid leaking mocked location into other tests
try {
Object.defineProperty(window, 'location', {
value: originalLocation,
configurable: true,
});
} catch (e) {
// best-effort restore; if restore fails don't throw from test helper
}
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open file
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
and locate renderWithRouter definition at lines 132-146. The helper sets window.location
via Object.defineProperty(window, 'location', ...) (lines 134-138) and then calls
render(...).
2. Run the jest test file (e.g., yarn test
src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx). The first test
that calls renderWithRouter is `test('generates a new form_data param when none is
available', ...)` (starts at line ~149). renderWithRouter will set window.location to {
pathname, search } and return without restoring the original window.location.
3. A later test in the same file, such as `test('doesnt call replaceState when pathname is
not /explore', ...)` (appears after the previous tests), expects window.location to be
readable or manipulated in a specific way (it spies on window.history.replaceState).
Because window.location was redefined and not restored, this can change the value seen by
subsequent tests and cause unexpected behavior or flakiness.
4. Observe flaky failures or unexpected assertions in tests that read or use
window.location/window.history across multiple renderWithRouter usages in this file.
Restoring the original window.location after renderWithRouter (as in the improved_code)
prevents the mocked location from leaking between tests. If the test helper intentionally
isolates location for an individual test, the improved code preserves that isolation and
then restores the global to avoid cross-test pollution.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
**Line:** 134:146
**Comment:**
*Logic Error: `renderWithRouter` overwrites `window.location` without preserving or restoring the original value, which can leak state between tests and break other browser APIs; save the original `window.location` and restore it after calling `render`.
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>
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -571,6 +571,29 @@ function ExploreViewContainer(props) {
}
}
+ // Automatically set axis title margins when titles are added or removed
+ if (changedControlKeys.includes('x_axis_title')) {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = props.controls.x_axis_title_margin?.value ?? 0;
+
+ if (xAxisTitle && currentMargin < 30) {
+ props.actions.setControlValue('x_axis_title_margin', 30);
+ } else if (!xAxisTitle && currentMargin !== 0) {
+ props.actions.setControlValue('x_axis_title_margin', 0);
+ }
+ }
+
+ if (changedControlKeys.includes('y_axis_title')) {
+ const yAxisTitle = props.controls.y_axis_title?.value || '';
+ const currentMargin = props.controls.y_axis_title_margin?.value ?? 0;
+
Review Comment:
**Suggestion:** Logic error: the code uses `currentMargin < 30` to decide when to auto-set the margin to 30, which will overwrite any smaller user-customized non-zero margins (e.g., 10 or 20). Per the intended behavior, the margin should only be auto-set when it is exactly 0 (the default), not whenever it is less than 30. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Overrides user axis-margin preferences in Explore controls.
- ❌ X axis title margin unexpectedly changes when edited.
- ❌ Y axis title margin unexpectedly changes when edited.
- ⚠️ Affects chart editing UX in ExploreViewContainer.
```
</details>
```suggestion
if (xAxisTitle && currentMargin === 0) {
props.actions.setControlValue('x_axis_title_margin', 30);
} else if (!xAxisTitle && currentMargin !== 0) {
props.actions.setControlValue('x_axis_title_margin', 0);
}
}
if (changedControlKeys.includes('y_axis_title')) {
const yAxisTitle = props.controls.y_axis_title?.value || '';
const currentMargin = props.controls.y_axis_title_margin?.value ?? 0;
if (yAxisTitle && currentMargin === 0) {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Explore view and load a chart editor (ExploreViewContainer at
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx). The controls
change handling runs in the useEffect that computes changedControlKeys and contains the
axis-margin logic (new logic is located at lines 574-594 in the PR hunk).
2. In the Controls panel (ConnectedControlPanelsContainer rendered in
ExploreViewContainer), set a non-zero custom margin for the X axis title control (the
control key is x_axis_title_margin). This writes a non-zero value (e.g., 10) into
props.controls.x_axis_title_margin.value.
3. Add or change the X axis title (update x_axis_title). The change causes props.controls
to update; the useEffect detects changedControlKeys includes 'x_axis_title' and executes
the block at lines 575-583. The code reads currentMargin and evaluates `currentMargin <
30` at line 579.
4. Because the condition uses `< 30`, the earlier user-chosen margin (10) satisfies the
condition and is overwritten by props.actions.setControlValue('x_axis_title_margin', 30)
at line 580. The user's deliberate smaller margin is lost. (Same flow for the Y axis with
lines 586-593.)
Note: This reproduces the exact behavior in the added block (lines 574-594). The change
from `< 30` to `=== 0` prevents overwriting user-custom values and only auto-sets when
margin is exactly the default 0.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
**Line:** 579:589
**Comment:**
*Logic Error: Logic error: the code uses `currentMargin < 30` to decide when to auto-set the margin to 30, which will overwrite any smaller user-customized non-zero margins (e.g., 10 or 20). Per the intended behavior, the margin should only be auto-set when it is exactly 0 (the default), not whenever it is less than 30.
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>
--
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] chore(charts): echars left padding too big and automation of title [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2676841492
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -559,6 +559,29 @@ function ExploreViewContainer(props) {
}
}
+ // Automatically set axis title margins when titles are added or removed
+ if (changedControlKeys.includes('x_axis_title')) {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = props.controls.x_axis_title_margin?.value ?? 0;
Review Comment:
Is this a neccessary change here? I am worried this might break in some edge cases. If we want this feature, i think we need more extensive tests(maybe e2e as well)
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,181 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+describe('Automatic axis title margin adjustment', () => {
Review Comment:
Let's not use describe blocks and flatten the test structure
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -190,3 +194,149 @@ describe('transformNegativeLabelsPosition', () => {
expect((result as any)[4].label).toBe(undefined);
});
});
+
+describe('getPadding', () => {
Review Comment:
Let's not use describe blocks and flatten the test structure
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #36993:
URL: https://github.com/apache/superset/pull/36993
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2718798805
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -474,3 +474,210 @@ test('shows error indicator with function labels', async () => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+
+function setupTableChartControlPanel() {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+}
+
+test('automatic axis title margin adjustment sets X axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: '' },
+ x_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ const { rerender } = renderWithRouter({ initialState: customState });
+
+ await waitFor(() => {
+ rerender(
+ <MemoryRouter initialEntries={[defaultPath]}>
+ <Route path={defaultPath}>
+ <ExploreViewContainer />
+ </Route>
+ </MemoryRouter>,
+ );
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('automatic axis title margin adjustment sets Y axis margin to 30 when title is added and margin is 0', async () => {
+ setupTableChartControlPanel();
+ try {
+ const customState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_title: { value: '' },
+ y_axis_title_margin: { value: 0 },
+ },
+ },
+ };
+
+ renderWithRouter({ initialState: customState });
+
+ // Verify initial state renders without errors
+ await waitFor(() => {
+ expect(
+ screen.queryByTestId('query-error-tooltip-trigger'),
+ ).not.toBeInTheDocument();
+ });
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "LuisSanchez (via GitHub)" <gi...@apache.org>.
LuisSanchez commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3786982424
Tests were updated, no changes in current logic of the 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] chore(charts): echarts left padding too big and automation of title [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36993:
URL: https://github.com/apache/superset/pull/36993#issuecomment-3785929614
⚠️ **DEPRECATED WORKFLOW** ⚠️
@sadpandajoe This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "🎪 trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/21259521456). 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