-
Notifications
You must be signed in to change notification settings - Fork 15.9k
feat(matrixify): replace single toggle with separate horizontal/vertical layout controls #35067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Adds configurable devserverHost parameter with CLI and env var support - Supports WEBPACK_DEVSERVER_HOST and WEBPACK_DEVSERVER_PORT environment variables - Maintains CLI argument precedence: --devserverHost > WEBPACK_DEVSERVER_HOST > 127.0.0.1 - Updates allowedHosts to support .local domains for LAN development - Removes docker-compose localhost binding restriction for external access - Follows 2025 framework standards: secure by default, easy opt-in for network access 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…cal layout controls 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 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Status |
---|---|---|
Over-permissive development server host access ▹ view | ✅ Fix detected |
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/src/utils/matrixifyUtils.ts | ✅ |
superset-frontend/src/explore/controlUtils/getSectionsToRender.ts | ✅ |
superset-frontend/packages/superset-ui-chart-controls/src/sections/matrixify.tsx | ✅ |
superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx | ✅ |
superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts | ✅ |
superset-frontend/src/explore/controlPanels/sections.tsx | ✅ |
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx | ✅ |
superset-frontend/src/components/Chart/ChartRenderer.jsx | ✅ |
superset-frontend/src/explore/components/ExploreChartPanel/index.tsx | ✅ |
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx | ✅ |
superset-frontend/webpack.config.js | ✅ |
superset/config.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
hot: true, | ||
host: devserverHost, | ||
port: devserverPort, | ||
allowedHosts: ['localhost', '.localhost', '127.0.0.1', '::1', '.local'], |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
df28911
to
8fffed7
Compare
🎪 Showtime deployed environment on GHA for 8fffed7 • Environment: http://54.190.0.29:8080 (admin/admin) |
1f97008
to
8fffed7
Compare
🎪 Showtime deployed environment on GHA for 16cfd53 • Environment: http://44.249.152.151:8080 (admin/admin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of nits!
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - with minor things
|
||
// Validate dimension is selected when visible | ||
const dimensionValidator = (value: any) => { | ||
if (!value?.dimension) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an option to avoid using any? Or does the value have a multitude of types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a good way to avoid this one
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx
Outdated
Show resolved
Hide resolved
visibility: ({ controls }) => | ||
(controls?.matrixify_mode_rows?.value || | ||
controls?.matrixify_mode_columns?.value) !== undefined, | ||
controls?.matrixify_enable_horizontal_layout?.value === true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controls?.matrixify_enable_horizontal_layout?.value === true, | |
controls?.matrixify_enable_horizontal_layout?.value, |
building upon #34526, here I'm replacing the single "Enable Matrixify" checkbox with two separate controls for better clarity:
This provides clearer user guidance about what each layout type does and allows independent control of row vs column matrix layouts.
Key changes: