#2213 Table toolbar should not be shown for single select tables #2214

mikieyx · 2024-10-17T22:51:22Z

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Signed-off-by: Michael Pavlik <[email protected]>

…sion-builder-chars

Signed-off-by: Michael Pavlik <[email protected]>

…into expression-builder-chars

…an to single select Signed-off-by: Michael Pavlik <[email protected]>

Signed-off-by: Michael Pavlik <[email protected]>

…as into no-tabletoolbar-singletable

Signed-off-by: Michael Pavlik <[email protected]>

…as into no-tabletoolbar-singletable

Signed-off-by: Michael Pavlik <[email protected]>

…as into no-tabletoolbar-singletable

nmgokhale · 2024-10-29T23:17:17Z

...as_modules/common-canvas/src/common-properties/components/flexible-table/flexible-table.scss

@@ -39,6 +39,11 @@ $row-left-padding: $spacing-02;
 	height: $spacing-07;
 }

+.single-table {


This class isn't used anywhere other than this file

I removed it on accident when merging. I used it to resolve the bug where the add value button would switch location depending on if the table was in focus

Rename to single-row-selection-table

Signed-off-by: Michael Pavlik <[email protected]>

nmgokhale · 2024-10-30T23:52:40Z

...dules/common-canvas/src/common-properties/components/virtualized-table/virtualized-table.jsx

+						className="delete-button"
+						hasIconOnly
+						onClick={this.props.deleteRow}
+						renderIcon={TrashCan}


Please add iconDescription for delete button

Hey Neha, I removed iconDescription because it would fail tests due to the fact that the icon description is considered to be part of the text. Let me know if I should just remove icon description or to modify the test cases.

@mikieyx You should add iconDescription and modify test cases.

nmgokhale · 2024-10-30T23:54:06Z

...dules/common-canvas/src/common-properties/components/virtualized-table/virtualized-table.jsx

@@ -414,6 +415,18 @@ class VirtualizedTable extends React.Component {
 						readOnly={this.props.readOnly}
 					/>
 				</div>);
+			} else {
+				trashOption = (


Rename this to deleteOption

nmgokhale · 2024-10-30T23:58:50Z

canvas_modules/harness/cypress/e2e/properties/properties-tooltips.cy.js

-		cy.toggleCommonPropertiesSidePanel();
-		cy.openPropertyDefinition("summaryPanel_paramDef.json");
-	});
+// describe("Test if tips show up for the summary table values", function() {


Enable all commented tests in this file

nmgokhale · 2024-10-31T00:35:16Z

canvas_modules/harness/test_resources/parameterDefs/structurelisteditor_paramDef.json

@@ -1216,7 +1216,7 @@
      },
      {
        "complex_type_ref": "inlineEditingTableError",
-        "row_selection": "single",
+        "row_selection": "multiple-edit",


This change is not needed. You can revert this to "single". Table looks odd because there is no header for column which has delete icons.

Take a look at how the header is added for subpanel column here - https://github.com/elyra-ai/canvas/blob/main/canvas_modules/common-canvas/src/common-properties/controls/abstract-table.jsx#L705

You should add a similar header for single select tables which will have a delete rows column.

I switched from single to multi select because single tables no longer have the toolbar which means that single tables can no longer move rows. The table has moveable rows set to true. I discussed this with Matt and Jesus, and they mentioned to switch it to multi select to ensure test coverage.

nmgokhale · 2024-10-31T00:35:28Z

...s_modules/common-canvas/__tests__/test_resources/paramDefs/structurelisteditor_paramDef.json

@@ -1216,7 +1216,7 @@
      },
      {
        "complex_type_ref": "inlineEditingTableError",
-        "row_selection": "single",
+        "row_selection": "multiple-edit",


This change is not needed. You can revert this to "single".

I switched from single to multi select because single tables no longer have the toolbar which means that single tables can no longer move rows. The table has moveable rows set to true. I discussed this with Matt and Jesus, and they mentioned to switch it to multi select to ensure test coverage.

nmgokhale · 2024-10-31T00:39:02Z

canvas_modules/common-canvas/src/common-properties/components/flexible-table/flexible-table.jsx

@@ -523,7 +523,8 @@ class FlexibleTable extends React.Component {

 		const containerClass = this.props.showHeader ? "properties-ft-container-absolute " : "properties-ft-container-absolute-noheader ";
 		const messageClass = (!this.props.messageInfo) ? containerClass + STATES.INFO : containerClass;
-		const ftHeaderClassname = "properties-ft-table-header";
+		const ftHeaderClassname = classNames("properties-ft-table-header",
+			{ "single-table": this.props.rowSelection === ROW_SELECTION.SINGLE });


Rename to single-row-selection-table

nmgokhale · 2024-10-31T00:39:16Z

...as_modules/common-canvas/src/common-properties/components/flexible-table/flexible-table.scss

@@ -39,6 +39,11 @@ $row-left-padding: $spacing-02;
 	height: $spacing-07;
 }

+.single-table {


Rename to single-row-selection-table

nmgokhale · 2024-10-31T00:45:39Z

When I click on Delete icon, row gets selected and it shows the inline editable fields briefly before deleting the row -

Screen.Recording.2024-10-30.at.5.42.59.PM.mov

Is it possible to not select the row when delete icon is clicked?

mikieyx · 2024-10-31T19:45:43Z

When I click on Delete icon, row gets selected and it shows the inline editable fields briefly before deleting the row -

Screen.Recording.2024-10-30.at.5.42.59.PM.mov

Is it possible to not select the row when delete icon is clicked?

When you click on the delete icon, it selects the row and it triggers the removeSelected() which removes the selected row. In this situation it first detects that the row is selected which pops open the inline editable fields before deleting. I'll try to take a closer look.

Signed-off-by: Michael Pavlik <[email protected]>

…as into no-tabletoolbar-singletable

nmgokhale · 2024-11-04T23:08:56Z

canvas_modules/common-canvas/src/common-properties/controls/abstract-table.jsx

@@ -703,6 +706,8 @@ export default class AbstractTable extends React.Component {
 		if (this.props.control.childItem && !this.isReadonlyTable()) {
 			// set to specific size. Exclude this column from resizing.
 			headers.push({ "key": "subpanel", "label": "", "width": TABLE_SUBPANEL_BUTTON_WIDTH, "staticWidth": true });
+		} else if (this.props.control.rowSelection === ROW_SELECTION.SINGLE) {


Remove else and just add if condition.
This code fails if the table has edit icon and delete icon both, there's only 1 table header.
To test this case, add "row_selection": "single", here - https://github.com/elyra-ai/canvas/blob/main/canvas_modules/harness/test_resources/parameterDefs/structurelisteditor_paramDef.json#L973

There are 7 columns but only 6 headers in the table.

Signed-off-by: Michael Pavlik <[email protected]>

…as into no-tabletoolbar-singletable

nmgokhale · 2024-11-05T19:32:22Z

In expression builder table, there's no header for delete column -

mikieyx · 2024-11-06T16:53:15Z

In expression builder table, there's no header for delete column -

I implemented the logic to add a column for the in abstract table. Abstract table uses flexible table and the expression builder also uses flexible table. I'll try to add the a column for the delete button in flexible table or even virtualized table (flexible table uses virtualized table)

matthoward366 · 2024-11-06T17:33:51Z

@mikieyx The problem is in the expression builder there should be no delete column.

@nmgokhale do you remember how we controlled this with single select before?

mikieyx · 2024-11-06T17:53:13Z

@mikieyx The problem is in the expression builder there should be no delete column.

@nmgokhale do you remember how we controlled this with single select before?

I can add a condition that only adds the delete icon if it's a read only table and then pass in the readOnly prop as true into the flexible table in expression-select-field-function.jsx

nmgokhale · 2024-11-06T20:39:34Z

@nmgokhale do you remember how we controlled this with single select before?

createTable() in abstract-table adds a table toolbar on a table. For expression field and functions tables, it never called createTable() method. So there was no table toolbar for expression tables.
With the addition of single select check in virtualized-table, delete icon started showing up in expression tables.

I can add a condition that only adds the delete icon if it's a read only table and then pass in the readOnly prop as true into the flexible table in expression-select-field-function.jsx

@mikieyx readOnly prop is not meant to show/hide delete icon in the table.
How about we check for tableLabel and if its fieldsTableLabel, valuesTableLabel or functionsTableLabel, don't show delete icon?

… for expression tables Signed-off-by: Michael Pavlik <[email protected]>

nmgokhale · 2024-11-07T19:09:52Z

...dules/common-canvas/src/common-properties/components/virtualized-table/virtualized-table.jsx

+			} else if (this.props.rowSelection === ROW_SELECTION.SINGLE && !(this.props.tableLabel === fieldsTableLabel ||
+				this.props.tableLabel === valuesTableLabel || this.props.tableLabel === functionsTableLabel
+			)) {
+				const toolTip = "Delete";


This tooltip should be translated

nmgokhale · 2024-11-07T19:10:51Z

...dules/common-canvas/src/common-properties/components/virtualized-table/virtualized-table.jsx

+							className="delete-button"
+							hasIconOnly
+							onClick={this.props.deleteRow}
+							renderIcon={TrashCan}


Add iconDescription here

nmgokhale · 2024-11-07T19:11:31Z

...dules/common-canvas/src/common-properties/components/virtualized-table/virtualized-table.jsx

@@ -414,6 +418,29 @@ class VirtualizedTable extends React.Component {
 						readOnly={this.props.readOnly}
 					/>
 				</div>);
+			} else if (this.props.rowSelection === ROW_SELECTION.SINGLE && !(this.props.tableLabel === fieldsTableLabel ||


Please add a comment here -
"Don't show delete icon for tables in expression builder"

nmgokhale · 2024-11-07T19:22:42Z

@mikieyx I am seeing extra width for Delete column -
Edit column and Delete column should have same width.

Wondering if this is because tooltip-container is outside the parent <div> for delete column.

mikieyx · 2024-11-07T20:18:24Z

@nmgokhale I dug into it, and it seems like the delete icons are not being connected to the right column. The headers are add in abstract table, while the delete icon is created in virtualized table. Abstract tables uses flexible table which uses virtualized table.

Signed-off-by: Michael Pavlik <[email protected]>

…as into no-tabletoolbar-singletable

Signed-off-by: Michael Pavlik <[email protected]>

nmgokhale · 2024-11-13T18:52:49Z

...dules/common-canvas/src/common-properties/components/virtualized-table/virtualized-table.jsx

@@ -414,6 +413,7 @@ class VirtualizedTable extends React.Component {
 						readOnly={this.props.readOnly}
 					/>
 				</div>);
+			// Don't show delete icon for tables in expression builder


remove this comment. This is no longer applicable now after adding delete icon in abstract-table

nmgokhale · 2024-11-13T18:55:08Z

canvas_modules/common-canvas/locales/common-properties/locales/eo.json

@@ -112,5 +112,6 @@
  "label.indicator.optional": "[Esperanto~(optional)~~~~~~eo]",
  "slider.numberInput.label": "[Esperanto~Slider number input~~~~~eo]",
  "editorForm.tabList.label": "[Esperanto~Primary Tabs~~~~eo]",
-  "subTabs.tabList.label": "[Esperanto~Tab List~~~~eo]"
+  "subTabs.tabList.label": "[Esperanto~Tab List~~~~eo]",
+  "table.deleteiIcon.tooltip": "[Esperanto~Delete~~~~eo]"


Typo in deleteiIcon

nmgokhale · 2024-11-13T18:56:09Z

canvas_modules/common-canvas/__tests__/common-properties/panels/summary-test.js

-		let deleteButton = tableToolbar.find("button.properties-action-delete");
-		expect(deleteButton).to.have.length(1);
-		deleteButton.simulate("click");
+		// let tableToolbar = tableWrapper.find("div.properties-table-toolbar");


Remove this comment

nmgokhale · 2024-11-13T18:57:24Z

...dules/common-canvas/src/common-properties/components/virtualized-table/virtualized-table.jsx

@@ -543,6 +543,7 @@ VirtualizedTable.propTypes = {
 		PropTypes.number.isRequired
 	]),
 	onRowDoubleClick: PropTypes.func,
+	deleteRow: PropTypes.func,


This isn't used anywhere

Signed-off-by: Michael Pavlik <[email protected]>

nmgokhale

Looks good 👍

Michael Pavlik and others added 20 commits August 27, 2024 09:55

Fixed expression char bug

a1410a5

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'main' of https://github.com/elyra-ai/canvas into expres…

b98b423

…sion-builder-chars

fixed the bug where you couldn't save work from expression builder

4851d2f

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'main' into expression-builder-chars

dd54411

removed unnecessary log statement

44f6580

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'expression-builder-chars' of github.com:mikieyx/canvas …

13f5c36

…into expression-builder-chars

Removed the table toolbar for single select tables, and added trash c…

520fb3c

…an to single select Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'main' into no-tabletoolbar-singletable

15f1132

eslint issue

cdbbaab

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'no-tabletoolbar-singletable' of github.com:mikieyx/canv…

1831514

…as into no-tabletoolbar-singletable

fixing test cases that are affected

87fbdd4

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'main' into no-tabletoolbar-singletable

bb66b18

adding and migrating structurelisteditor tests

435a2f5

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'no-tabletoolbar-singletable' of github.com:mikieyx/canv…

fc1dd78

…as into no-tabletoolbar-singletable

Merge branch 'main' into no-tabletoolbar-singletable

ebbdac2

small change to run build again

7889ede

Signed-off-by: Michael Pavlik <[email protected]>

small change to trigger build

cbdd677

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'main' into no-tabletoolbar-singletable

8acf300

Fixing cypress test

a36a9e8

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'no-tabletoolbar-singletable' of github.com:mikieyx/canv…

3f82efb

…as into no-tabletoolbar-singletable

mikieyx requested a review from nmgokhale October 29, 2024 16:52

mikieyx added 2 commits October 29, 2024 12:54

Merge branch 'main' into no-tabletoolbar-singletable

90eeeec

Merge branch 'main' into no-tabletoolbar-singletable

a48b761

nmgokhale reviewed Oct 29, 2024

View reviewed changes

mikieyx and others added 2 commits October 30, 2024 09:21

Merge branch 'main' into no-tabletoolbar-singletable

ebce123

adding in single-table class name for single tables

225b361

Signed-off-by: Michael Pavlik <[email protected]>

nmgokhale reviewed Oct 31, 2024

View reviewed changes

revisions

198d6c8

Signed-off-by: Michael Pavlik <[email protected]>

Michael Pavlik and others added 2 commits October 31, 2024 13:43

Merge branch 'no-tabletoolbar-singletable' of github.com:mikieyx/canv…

5aba785

…as into no-tabletoolbar-singletable

Merge branch 'main' into no-tabletoolbar-singletable

d184cd1

nmgokhale reviewed Nov 4, 2024

View reviewed changes

Michael Pavlik added 2 commits November 5, 2024 09:31

fixing delete button column

4481304

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'no-tabletoolbar-singletable' of github.com:mikieyx/canv…

9fe6968

…as into no-tabletoolbar-singletable

mikieyx and others added 2 commits November 6, 2024 15:38

Merge branch 'main' into no-tabletoolbar-singletable

d93d2c8

add tool tip for delete icon + conditions so delete icon is not shown…

808590b

… for expression tables Signed-off-by: Michael Pavlik <[email protected]>

nmgokhale reviewed Nov 7, 2024

View reviewed changes

mikieyx and others added 6 commits November 7, 2024 12:18

Merge branch 'main' into no-tabletoolbar-singletable

3ff129c

Small changes

ca80817

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'main' into no-tabletoolbar-singletable

d7af953

implemented trash icon in abstract table

9140d6f

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'no-tabletoolbar-singletable' of github.com:mikieyx/canv…

edbbc99

…as into no-tabletoolbar-singletable

rewriting test cases

4af4dc6

Signed-off-by: Michael Pavlik <[email protected]>

nmgokhale reviewed Nov 13, 2024

View reviewed changes

minor changes

9afdbc0

Signed-off-by: Michael Pavlik <[email protected]>

nmgokhale approved these changes Nov 13, 2024

View reviewed changes

nmgokhale merged commit 3bebb81 into elyra-ai:main Nov 13, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2213 Table toolbar should not be shown for single select tables #2214

#2213 Table toolbar should not be shown for single select tables #2214

mikieyx commented Oct 17, 2024

nmgokhale Oct 29, 2024

mikieyx Oct 30, 2024 •

edited

Loading

nmgokhale Oct 31, 2024

nmgokhale Oct 30, 2024

mikieyx Oct 31, 2024

nmgokhale Nov 4, 2024

nmgokhale Oct 30, 2024

nmgokhale Oct 30, 2024

nmgokhale Oct 31, 2024

mikieyx Oct 31, 2024 •

edited

Loading

nmgokhale Oct 31, 2024

mikieyx Oct 31, 2024

nmgokhale Oct 31, 2024

nmgokhale Oct 31, 2024

nmgokhale commented Oct 31, 2024

mikieyx commented Oct 31, 2024

nmgokhale Nov 4, 2024

nmgokhale commented Nov 5, 2024

mikieyx commented Nov 6, 2024

matthoward366 commented Nov 6, 2024

mikieyx commented Nov 6, 2024

nmgokhale commented Nov 6, 2024 •

edited

Loading

nmgokhale Nov 7, 2024

nmgokhale Nov 7, 2024

nmgokhale Nov 7, 2024

nmgokhale commented Nov 7, 2024

mikieyx commented Nov 7, 2024 •

edited

Loading

nmgokhale Nov 13, 2024

nmgokhale Nov 13, 2024

nmgokhale Nov 13, 2024

nmgokhale Nov 13, 2024

nmgokhale left a comment

#2213 Table toolbar should not be shown for single select tables #2214

#2213 Table toolbar should not be shown for single select tables #2214

Conversation

mikieyx commented Oct 17, 2024

Choose a reason for hiding this comment

mikieyx Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikieyx Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmgokhale commented Oct 31, 2024

mikieyx commented Oct 31, 2024

Choose a reason for hiding this comment

nmgokhale commented Nov 5, 2024

mikieyx commented Nov 6, 2024

matthoward366 commented Nov 6, 2024

mikieyx commented Nov 6, 2024

nmgokhale commented Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmgokhale commented Nov 7, 2024

mikieyx commented Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmgokhale left a comment

Choose a reason for hiding this comment

mikieyx Oct 30, 2024 •

edited

Loading

mikieyx Oct 31, 2024 •

edited

Loading

nmgokhale commented Nov 6, 2024 •

edited

Loading

mikieyx commented Nov 7, 2024 •

edited

Loading