machines: fix issue with dialogs that are nested inside PF4 tables don't close when submitted #13488

KKoukiou · 2020-02-04T13:32:22Z

No description provided.

pkg/lib/cockpit-components-table.jsx

martinpitt · 2020-02-04T15:29:23Z

pkg/lib/cockpit-components-table.jsx

@@ -161,6 +164,7 @@ export class ListingTable extends React.Component {
        // We need the following because of: https://github.com/patternfly/patternfly-react/issues/3090
        if (props.cells.some(cell => typeof (cell.title) != 'string' || cell.title.toLowerCase() == 'id' || !cell.title))
            tableBodyProps.rowKey = ({ rowData, rowIndex }) => rowIndex;


Using an ID for each row seems strange, though. This is the place that sets the keys for the rows, and obviously rowIndex is an useless value for a row key. IMHO it would be better to make rowKey a mandatory property of a table row component, and let the caller set it. Perhaps it could have a default like a hash value of rowData, but I'm not sure if that's possible/easy to do in JS.

martinpitt · 2020-02-04T15:30:33Z

pkg/machines/components/vmDisksTab.jsx

@@ -186,7 +186,7 @@ class VmDisksTab extends React.Component {
                );
                columns.push({ title: diskActions });
            }
-            return { columns };
+            return { columns, id: disk.target };


Passing a key: (or rowKey:) here would IMHO make more sense. keys can have a much more flexible content than IDs.

Passing a rowKey to the row properties won't make PF use it unfortunately.
I need to write some clean reproducer and submit an issue to PF, because the current way to have custom row keys (something else than index) is not very nice actually :/

Check my updated PR for implementation details.

martinpitt

I only looked at the first commit for now, this needs more rework I'm afraid. Can you please revise the commit message? It has a lot of typos and grammar errors.

Thanks! Sorry that you have to wade through this brokenness :/

martinpitt · 2020-02-05T13:30:16Z

pkg/lib/cockpit-components-table.jsx

@@ -41,7 +41,8 @@ import './listing.less';
 * - columns: { title: string, header: boolean, sortable: boolean }[] or string[]
 * - rows: {
 *      columns: (React.Node or string)[],
- *      extraClasses: string[]
+ *      extraClasses: string[],
+ *      rowKey: ({ rowData: IRowData, rowIndex: Number }) => string | if not specified the rowIndex will be used as each row's key,


I suppose "IRowData" is a typo? Would be good to give an example of its structure, as by our IRC conversation this is an utterly strange object.

martinpitt · 2020-02-05T13:31:37Z

pkg/systemd/hwinfo.jsx

@@ -252,6 +252,7 @@ class HardwareInfo extends React.Component {

            pci = (
                <ListingTable caption={ _("PCI") }
+                    rowKey={({ rowData, rowIndex }) => rowData.slot.title}


If I understand this correctly, then .slot will change if the title is translated. And of course it's not entirely obvious what happens if you translate _("Slot") into a language which doesn't have lower case and such.. 😭

martinpitt · 2020-02-05T13:32:25Z

pkg/systemd/hwinfo.jsx

@@ -268,6 +269,7 @@ class HardwareInfo extends React.Component {
        if (this.props.info.memory.length > 0) {
            memory = (
                <ListingTable caption={ _("Memory") }
+                    rowKey={({ rowData, rowIndex }) => rowData.id.title}


Same here, probably needs to use rowData[_("ID").toLowerCase()] or something similarly ugly

…gTable A consumer of ListingTable component can now pass a unique key to the `props` object of each row, which will be then internally used by React as the row's key. Start using this property for the Tables in Hardware Info page, so that we can drop the workaround for wrong key assignment in the ListingTable implementation. Closes cockpit-project#13488

… a disk Specify a key in the props property so that a unique key is used for each row of the PF4 Table. Related to https://bugzilla.redhat.com/show_bug.cgi?id=1791543

…a NIC Specify a key in props property so that a unique key is used for each row of the PF4 Table. Note: As id, the {mac}-{address.bus}-{address.slot} is used. When the NIC does not have an address element in the XML we use the {mac}-{networkIndex} combination, which is though problematic if more interfaces have the same MAC. Related to https://bugzilla.redhat.com/show_bug.cgi?id=1791543

This will prevent consumers of ListingTable to have rows with keys that are just row indexes. Fix all ListingTables occurances which didn't specify that key attribute.

martinpitt

Since the fourth commit makes it mandatory, there should be a follow-up PR that drops the fallback to rowIndex. However, that doesn't need to block this PR (and hence the release). Thanks!

martinpitt · 2020-02-05T15:42:59Z

pkg/lib/cockpit-components-table.jsx

-        // We need the following because of: https://github.com/patternfly/patternfly-react/issues/3090
-        if (props.cells.some(cell => typeof (cell.title) != 'string' || cell.title.toLowerCase() == 'id' || !cell.title))
-            tableBodyProps.rowKey = ({ rowData, rowIndex }) => rowIndex;
+        const tableBodyProps = { rowKey: ({ rowData, rowIndex }) => (rowData.props && rowData.props.key) ? rowData.props.key : rowIndex };


I'd rather not put a fallback here. rowIndex is wrong, and I'd rather have a React warning about a missing key, so that the issue is not silently covered.

…gTable A consumer of ListingTable component can now pass a unique key to the `props` object of each row, which will be then internally used by React as the row's key. Start using this property for the Tables in Hardware Info page, so that we can drop the workaround for wrong key assignment in the ListingTable implementation. Closes #13488

KKoukiou added the release-blocker Targetted for next release label Feb 4, 2020

KKoukiou mentioned this pull request Feb 4, 2020

machines: Fix disk remove dialog staying open after removal #13436

Closed

martinpitt requested changes Feb 4, 2020

View reviewed changes

KKoukiou force-pushed the fix-table-keys branch from 94b40b8 to b83060e Compare February 5, 2020 11:11

KKoukiou requested a review from martinpitt February 5, 2020 12:42

martinpitt requested changes Feb 5, 2020

View reviewed changes

KKoukiou force-pushed the fix-table-keys branch from b83060e to ca50f9c Compare February 5, 2020 14:12

KKoukiou added 4 commits February 5, 2020 15:28

machines: fix issue with Disk Delete dialog not closing when deleting…

0e27359

… a disk Specify a key in the props property so that a unique key is used for each row of the PF4 Table. Related to https://bugzilla.redhat.com/show_bug.cgi?id=1791543

lib: Make the props property in in ListingTable a mandatory one

57895f6

This will prevent consumers of ListingTable to have rows with keys that are just row indexes. Fix all ListingTables occurances which didn't specify that key attribute.

KKoukiou force-pushed the fix-table-keys branch from ca50f9c to 57895f6 Compare February 5, 2020 14:32

KKoukiou requested a review from martinpitt February 5, 2020 15:05

martinpitt approved these changes Feb 5, 2020

View reviewed changes

KKoukiou mentioned this pull request Feb 5, 2020

machines: Support transient VMs #13241

Merged

martinpitt merged commit 11bd2c0 into cockpit-project:master Feb 5, 2020

KKoukiou deleted the fix-table-keys branch February 6, 2020 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

machines: fix issue with dialogs that are nested inside PF4 tables don't close when submitted #13488

machines: fix issue with dialogs that are nested inside PF4 tables don't close when submitted #13488

KKoukiou commented Feb 4, 2020

martinpitt Feb 4, 2020

martinpitt Feb 4, 2020

KKoukiou Feb 5, 2020

KKoukiou Feb 5, 2020

martinpitt left a comment

martinpitt Feb 5, 2020

martinpitt Feb 5, 2020

martinpitt Feb 5, 2020

martinpitt left a comment

martinpitt Feb 5, 2020

machines: fix issue with dialogs that are nested inside PF4 tables don't close when submitted #13488

machines: fix issue with dialogs that are nested inside PF4 tables don't close when submitted #13488

Conversation

KKoukiou commented Feb 4, 2020

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

martinpitt left a 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

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment