bito-code-review[bot] commented on code in PR #32958:
URL: https://github.com/apache/superset/pull/32958#discussion_r3717351615
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -208,7 +208,10 @@ const Chart = (props: ChartProps) => {
);
const isExpanded = useSelector(
(state: RootState) =>
- !!(state.dashboardState as JsonObject).expandedSlices?.[props.id],
+ !!(
+ (state.dashboardState as JsonObject).expandedSlices?.[props.id] ??
+ (state.dashboardState as JsonObject).expandAllSlices
Review Comment:
<!-- Bito Reply -->
The suggestion to add `expandAllSlices` to the `DashboardState` and
`DashboardStateShape` is valid and necessary to resolve the type inconsistency
identified in the review. The current implementation in `Chart.tsx` correctly
uses the nullish coalescing operator to check both the per-slice state and the
global `expandAllSlices` setting, which is the intended fix for the reported
issue.
**superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx**
```
const isExpanded = useSelector(
(state: RootState) =>
!!(
(state.dashboardState as JsonObject).expandedSlices?.[props.id] ??
(state.dashboardState as JsonObject).expandAllSlices
),
);
```
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx:
##########
@@ -146,17 +146,80 @@ test('should render a ChartContainer', () => {
expect(getByTestId('chart-container')).toBeInTheDocument();
});
-test('should render a description if it has one and isExpanded=true', () => {
- const { container } = setup(
- {},
- {
- dashboardState: {
- ...defaultState.dashboardState,
- expandedSlices: { [props.id]: true },
- },
- },
- );
- expect(container.querySelector('.slice_description')).toBeInTheDocument();
+describe('no description set', () => {
+ for (const sliceExpanded of [undefined, false, true]) {
+ for (const allExpanded of [undefined, false, true]) {
+ test(`should not render a description, expandedSlices=${sliceExpanded}
and expandAllSlices=${allExpanded}`, () => {
+ const { container } = setup(
+ {},
+ {
+ dashboardState: {
+ ...defaultState.dashboardState,
+ expandedSlices: { [props.id]: sliceExpanded },
+ expandAllSlices: allExpanded,
+ },
+ sliceEntities: {
+ ...sliceEntities,
+ slices: {
+ [queryId]: {
+ ...sliceEntities.slices[queryId],
+ description_markeddown: undefined,
Review Comment:
<!-- Bito Reply -->
The fix is appropriate. By updating the test to use the correct property
name `description_markdown` (matching the component's implementation), the test
now correctly validates the rendering logic instead of relying on a property
mismatch.
**superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx**
```
description_markdown: undefined,
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
