You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2025/09/09 08:31:52 UTC
[PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
mistercrunch opened a new pull request, #35067:
URL: https://github.com/apache/superset/pull/35067
Replace the single "Enable Matrixify" checkbox with two separate controls for better clarity:
- "Enable vertical layout (rows)" - for creating matrix rows
- "Enable horizontal layout (columns)" - for creating matrix columns
This provides clearer user guidance about what each layout type does and allows
independent control of row vs column matrix layouts.
Key changes:
- Split matrixify_enabled into matrixify_enable_vertical_layout and matrixify_enable_horizontal_layout
- Updated all references across components to use new layout controls
- Improved control panel organization with progressive disclosure
- Enhanced visibility logic to show relevant controls based on layout selection
- Added utility functions for checking layout states
- Removed problematic validators that were blocking form rendering
- Enabled MATRIXIFY feature flag for testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #35067:
URL: https://github.com/apache/superset/pull/35067#discussion_r2355043238
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx:
##########
@@ -82,24 +92,47 @@ const matrixifyControls: Record<string, SharedControlConfig<any>> = {};
const getValue = (key: string, defaultValue?: any) =>
form_data?.[key] ?? controls?.[key]?.value ?? defaultValue;
+ const selectionMode = getValue(
+ `matrixify_dimension_selection_mode_${axis}`,
+ 'members',
+ );
+
+ const isVisible =
+ controls?.[
+ `matrixify_enable_${axis === 'rows' ? 'vertical' : 'horizontal'}_layout`
Review Comment:
Can we put this in a small util for readability?
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -679,11 +680,54 @@ function ExploreViewContainer(props) {
return errorMessage;
}, [props.controls]);
+ // Error message for Data tab only (excludes matrixify controls)
+ const dataTabErrorMessage = useMemo(() => {
+ const controlsWithErrors = Object.values(props.controls).filter(
+ control =>
+ control.validationErrors &&
+ control.validationErrors.length > 0 &&
+ control.tabOverride !== 'matrixify', // Exclude matrixify controls from Data tab
+ );
+ if (controlsWithErrors.length === 0) {
+ return null;
+ }
+
+ const errorMessages = controlsWithErrors.map(
+ control => control.validationErrors,
+ );
+ const uniqueErrorMessages = [...new Set(errorMessages.flat())];
+
+ const errors = uniqueErrorMessages
+ .map(message => {
+ const matchingLabels = controlsWithErrors
+ .filter(control => control.validationErrors?.includes(message))
+ .map(control =>
+ typeof control.label === 'function'
+ ? control.label(props.exploreState)
+ : control.label,
+ );
+ return [matchingLabels, message];
+ })
+ .map(([labels, message]) => (
+ <div key={message}>
+ {labels.length > 1 ? t('Controls labeled ') : t('Control labeled ')}
+ <strong>{` ${labels.join(', ')}`}</strong>
+ <span>: {message}</span>
+ </div>
+ ));
+
+ let dataTabErrorMessage;
+ if (errors.length > 0) {
+ dataTabErrorMessage = <div style={{ textAlign: 'left' }}>{errors}</div>;
Review Comment:
Let's use `css` over `style` for consistency
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #35067:
URL: https://github.com/apache/superset/pull/35067#discussion_r2356159231
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx:
##########
@@ -82,24 +92,47 @@ const matrixifyControls: Record<string, SharedControlConfig<any>> = {};
const getValue = (key: string, defaultValue?: any) =>
form_data?.[key] ?? controls?.[key]?.value ?? defaultValue;
+ const selectionMode = getValue(
+ `matrixify_dimension_selection_mode_${axis}`,
+ 'members',
+ );
+
+ const isVisible =
+ controls?.[
+ `matrixify_enable_${axis === 'rows' ? 'vertical' : 'horizontal'}_layout`
+ ]?.value === true &&
+ controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions';
+
+ // Validate dimension is selected when visible
+ const dimensionValidator = (value: any) => {
+ if (!value?.dimension) {
Review Comment:
I don't think there is a good way to avoid this one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/superset/pull/35067#issuecomment-3293470742
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/17742947304) for [16cfd53](https://github.com/apache/superset/commit/16cfd53)
• **Environment:** http://44.249.152.151:8080 (admin/admin)
• **Lifetime:** 24h 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] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/superset/pull/35067#issuecomment-3271467279
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/17589008564) for [8fffed7](https://github.com/apache/superset/commit/8fffed7)
• **Environment:** http://54.190.0.29:8080 (admin/admin)
• **Lifetime:** 24h 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] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz merged PR #35067:
URL: https://github.com/apache/superset/pull/35067
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/superset/pull/35067#issuecomment-3271422614
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/17589008564) for [8fffed7](https://github.com/apache/superset/commit/8fffed7)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35067:
URL: https://github.com/apache/superset/pull/35067#discussion_r2332529803
##########
superset-frontend/webpack.config.js:
##########
@@ -568,7 +574,9 @@ if (isDevMode) {
},
historyApiFallback: true,
hot: true,
+ host: devserverHost,
port: devserverPort,
+ allowedHosts: ['localhost', '.localhost', '127.0.0.1', '::1', '.local'],
Review Comment:
### Over-permissive development server host access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The allowedHosts configuration is too permissive by including '.local' which allows any hostname ending with .local to access the dev server.
###### Why this matters
This could enable DNS rebinding attacks in development environments by allowing malicious hosts with .local TLD to connect to the development server.
###### Suggested change ∙ *Feature Preview*
```javascript
allowedHosts: ['localhost', '127.0.0.1', '::1'],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d89deb8c-6331-4d97-9476-0c1e4879ac72/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d89deb8c-6331-4d97-9476-0c1e4879ac72?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d89deb8c-6331-4d97-9476-0c1e4879ac72?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d89deb8c-6331-4d97-9476-0c1e4879ac72?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d89deb8c-6331-4d97-9476-0c1e4879ac72)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6d74dcf9-b766-427f-834c-029b00913c52 -->
[](6d74dcf9-b766-427f-834c-029b00913c52)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #35067:
URL: https://github.com/apache/superset/pull/35067#discussion_r2356160790
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx:
##########
@@ -82,24 +92,47 @@ const matrixifyControls: Record<string, SharedControlConfig<any>> = {};
const getValue = (key: string, defaultValue?: any) =>
form_data?.[key] ?? controls?.[key]?.value ?? defaultValue;
+ const selectionMode = getValue(
+ `matrixify_dimension_selection_mode_${axis}`,
+ 'members',
+ );
+
+ const isVisible =
+ controls?.[
+ `matrixify_enable_${axis === 'rows' ? 'vertical' : 'horizontal'}_layout`
+ ]?.value === true &&
+ controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions';
+
+ // Validate dimension is selected when visible
+ const dimensionValidator = (value: any) => {
+ if (!value?.dimension) {
+ return t('Dimension is required');
+ }
+ return false;
+ };
+
+ // Additional validation for topN mode
+ const validators = isVisible
+ ? [dimensionValidator, validateNonEmpty]
+ : [];
+
return {
datasource,
- selectionMode: getValue(
- `matrixify_dimension_selection_mode_${axis}`,
- 'members',
- ),
+ selectionMode,
topNMetric: getValue(`matrixify_topn_metric_${axis}`),
topNValue: getValue(`matrixify_topn_value_${axis}`),
topNOrder: getValue(`matrixify_topn_order_${axis}`),
formData: form_data,
+ validators,
};
},
visibility: ({ controls }) =>
+ controls?.[
+ `matrixify_enable_${axis === 'rows' ? 'vertical' : 'horizontal'}_layout`
+ ]?.value === true &&
controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions',
Review Comment:
Should be better with new util function!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/superset/pull/35067#issuecomment-3293429444
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/17742947304) for [16cfd53](https://github.com/apache/superset/commit/16cfd53)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(matrixify): replace single toggle with separate horizontal/vertical layout controls [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #35067:
URL: https://github.com/apache/superset/pull/35067#discussion_r2355051720
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx:
##########
@@ -82,24 +92,47 @@ const matrixifyControls: Record<string, SharedControlConfig<any>> = {};
const getValue = (key: string, defaultValue?: any) =>
form_data?.[key] ?? controls?.[key]?.value ?? defaultValue;
+ const selectionMode = getValue(
+ `matrixify_dimension_selection_mode_${axis}`,
+ 'members',
+ );
+
+ const isVisible =
+ controls?.[
+ `matrixify_enable_${axis === 'rows' ? 'vertical' : 'horizontal'}_layout`
+ ]?.value === true &&
+ controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions';
+
+ // Validate dimension is selected when visible
+ const dimensionValidator = (value: any) => {
+ if (!value?.dimension) {
+ return t('Dimension is required');
+ }
+ return false;
+ };
+
+ // Additional validation for topN mode
+ const validators = isVisible
+ ? [dimensionValidator, validateNonEmpty]
+ : [];
+
return {
datasource,
- selectionMode: getValue(
- `matrixify_dimension_selection_mode_${axis}`,
- 'members',
- ),
+ selectionMode,
topNMetric: getValue(`matrixify_topn_metric_${axis}`),
topNValue: getValue(`matrixify_topn_value_${axis}`),
topNOrder: getValue(`matrixify_topn_order_${axis}`),
formData: form_data,
+ validators,
};
},
visibility: ({ controls }) =>
+ controls?.[
+ `matrixify_enable_${axis === 'rows' ? 'vertical' : 'horizontal'}_layout`
+ ]?.value === true &&
controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions',
Review Comment:
maybe we can try to use enums for some of the string values like "vertical_layout" "horizontal_layout" "horizontal" etc
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx:
##########
@@ -256,9 +350,9 @@ matrixifyControls.matrixify_show_column_headers = {
description: t('Display headers for each column at the top of the matrix'),
default: true,
renderTrigger: true,
+ tabOverride: 'matrixify',
visibility: ({ controls }) =>
- (controls?.matrixify_mode_rows?.value ||
- controls?.matrixify_mode_columns?.value) !== undefined,
+ controls?.matrixify_enable_horizontal_layout?.value === true,
Review Comment:
```suggestion
controls?.matrixify_enable_horizontal_layout?.value,
```
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx:
##########
@@ -82,24 +92,47 @@ const matrixifyControls: Record<string, SharedControlConfig<any>> = {};
const getValue = (key: string, defaultValue?: any) =>
form_data?.[key] ?? controls?.[key]?.value ?? defaultValue;
+ const selectionMode = getValue(
+ `matrixify_dimension_selection_mode_${axis}`,
+ 'members',
+ );
+
+ const isVisible =
+ controls?.[
+ `matrixify_enable_${axis === 'rows' ? 'vertical' : 'horizontal'}_layout`
+ ]?.value === true &&
+ controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions';
+
+ // Validate dimension is selected when visible
+ const dimensionValidator = (value: any) => {
+ if (!value?.dimension) {
Review Comment:
Is there an option to avoid using any? Or does the value have a multitude of types?
--
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