michael-s-molina commented on code in PR #33146:
URL: https://github.com/apache/superset/pull/33146#discussion_r2222567364


##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -34,6 +34,51 @@ const config: ControlPanelConfig = {
       expanded: true,
       controlSetRows: [
         ['x_axis'],
+        [
+          {
+            name: 'xAxisSortByColumn',
+            config: {
+              type: 'SelectControl',
+              label: t('X axis sort by column'),
+              description: t(
+                'Column to use for ordering the waterfall based on a column',
+              ),
+              mapStateToProps: state => ({
+                choices: [
+                  ...(state.datasource?.columns || []).map(col => [
+                    col.column_name,
+                    col.column_name,
+                  ]),
+                ],
+                default: '',

Review Comment:
   Default shouldn't be empty given that when you clear the selection, it's 
creating an empty item in the select.



##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -34,6 +34,51 @@ const config: ControlPanelConfig = {
       expanded: true,
       controlSetRows: [
         ['x_axis'],
+        [
+          {
+            name: 'xAxisSortByColumn',
+            config: {
+              type: 'SelectControl',
+              label: t('X axis sort by column'),

Review Comment:
   Check the video that I linked in this PR about how this works on Bar chart. 
The label should be "X-axis sort by".



##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/transformProps.ts:
##########
@@ -452,14 +490,301 @@ export default function transformProps(
     series: barSeries,
   };
 
+  const processSeriesData = (options: EChartsOption) => {

Review Comment:
   Could you refactor the code and extract each commented section to functions 
in a `utils.ts` file to improve readability? Examples:
   ```
   // Handle subtotal styling
   // Handle total visibility
   // Handle orientation
   ...
   getFormattedAxisOptions
   getCrossFilteredSeries
   ...
   ```



##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -34,6 +34,51 @@ const config: ControlPanelConfig = {
       expanded: true,
       controlSetRows: [
         ['x_axis'],
+        [
+          {
+            name: 'xAxisSortByColumn',
+            config: {
+              type: 'SelectControl',
+              label: t('X axis sort by column'),
+              description: t(
+                'Column to use for ordering the waterfall based on a column',
+              ),
+              mapStateToProps: state => ({
+                choices: [
+                  ...(state.datasource?.columns || []).map(col => [
+                    col.column_name,
+                    col.column_name,
+                  ]),
+                ],
+                default: '',
+              }),
+              renderTrigger: false,
+              clearable: true,
+              visibility: ({ controls }) => Boolean(controls?.x_axis?.value),
+            },
+          },
+        ],
+        [
+          {
+            name: 'xAxisSortByColumnOrder',

Review Comment:
   Check the video that I linked in this PR about how this works on Bar chart. 
This control should be a checkbox called "X-axis sort ascending".



-- 
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-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org

Reply via email to