feat(Table): support inline editing in PF4 #1227

atiratree · 2019-01-22T13:17:45Z

Hi, I tried to tackle inline editing with reactabular. But it is still not perfect and a bit hacky.

My main goal was to prevent this behavior: kubevirt/web-ui-components#9, but other issues are still present.

inability to set a cursor to other edited cell (the cursor will firstly appear at the end and then it can be moved)
no possibility for real time validation (char by char)

This is a due to the reactabular architecture, where we cannot expect our cells to behave in react-like way and have to implement workarounds introduced by this PR. It is inspired by https://github.com/reactabular/react-edit and uses onBlur and autoFocus on input elements

There is one issue with gatsby? where the whole table page is refreshed and the table component is called by costructor. This happens on a dropdown selection in Editable table With Inline Edit Row

live site: https://1227-pr-patternfly-react-patternfly.surge.sh/patternfly-4/components/inlineedit

TODO:

Current Issues:

table borders misbehave in firefox due to https://bugzilla.mozilla.org/show_bug.cgi?id=688556 (Table: incorrect borders in Firefox patternfly#1520 )
gatsby refreshes on dropdown select, but doesn't happen when the same component is rendered without gatsby - possibly issue with gatsby

@priley86 @karelhala

atiratree · 2019-01-22T13:18:59Z

packages/patternfly-4/react-core/src/components/TextInput/TextInput.js

@@ -52,7 +52,7 @@ const defaultProps = {
  isDisabled: false,
  isReadOnly: false,
  type: 'text',
-  value: null,
+  value: undefined,


to allow uncontrolled components like TableTextInput

atiratree · 2019-01-22T13:21:05Z

packages/patternfly-4/react-table/internal/util.js

@@ -0,0 +1,8 @@
+// TODO: better sharing util components between modules


this is copied from @react-core internal. Does someone have any recommendations how to share utility classes between modules?

added #1232 for this. If it merges you can:

import { debounce } from '@patternfly/react-core';

atiratree · 2019-01-22T13:22:56Z

packages/patternfly-4/react-table/src/components/Table/Body.js

-      onClick: (event) => onRowClick(event, row, props)
+      row,
+      rowProps: props,
+      onMouseDown: (event) => {


onMouseDown is needed to register clicks before onChange/onBlur changes the row (which discards the onClick)

this seems like kind of a hack, but works. I wonder if proposing an upstream PR to reactabular in parallel to allow hooks into onChange onBlur would be a good idea?

Where would the hooks be? I am not sure something like this would help.
The idea of this is to get ahead of cell onBlur callbacks. Because of the cell's onBlur callback, the cell will be scrapped and then constructed again. And then we get a chance to fire callbacks (captured here) on our new instance.

it is a hack nevertheless :)

One note, can this function be moved outside of this component? So it would be easier to read the component when debugging?

atiratree · 2019-01-22T13:24:29Z

packages/patternfly-4/react-table/src/components/Table/Table.js

+};
+
+export const TableEditConfirmation = {
+  NONE: 'NONE', // TODO: do edit buttons make sense on the left?


Should we include more combinations of edit buttons? E.g. on the left?

This question is better to be asked any UX person @mcarrano any thoughts on this?

atiratree · 2019-01-22T13:25:27Z

packages/patternfly-4/react-table/src/components/Table/TableConfirmButtonsRow.js

+};
+
+// TODO: shouldComponentUpdate?
+// TableConfirmButtonsRow.shouldComponentUpdate = true;


not really sure yet if shouldComponentUpdate is needed? It was present in the pf3 component

As long as you are not trying to calculate re-renders for you we don't really need to use shouldComponentUpdate

atiratree · 2019-01-22T13:25:56Z

packages/patternfly-4/react-table/src/components/Table/examples/EditableTable.js

+            privateRepo: false
+          }
+          // isEditing: true,
+          // activeEditCell: 3


possibility to select editing programatically

coveralls · 2019-01-22T13:35:23Z

Pull Request Test Coverage Report for Build 4563

0 of 0 changed or added relevant lines in 0 files are covered.
31 unchanged lines in 3 files lost coverage.
Overall coverage increased (+2.08%) to 81.212%

Files with Coverage Reduction	New Missed Lines	%
packages/patternfly-4/react-core/src/components/Dropdown/Separator.js	1	80.0%
packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js	4	34.52%
packages/patternfly-4/react-core/src/helpers/util.ts	26	14.29%

Totals
Change from base Build 4547:	2.08%
Covered Lines:	3670
Relevant Lines:	4239

💛 - Coveralls

priley86 · 2019-01-22T15:53:27Z

Added issue 1228 for this.

priley86 · 2019-01-22T18:32:38Z

packages/patternfly-4/react-table/src/components/Table/RowWrapper.js

+      case TableEditConfirmation.ROW:
+        if (isEditing) {
+          editStyle = {
+            background: '#def3ff',


can you replace all color references with actual color tokens from @patternfly/react-tokens (ex consumption here )?

you can also find these and their default mappings here:
http://patternfly-react.surge.sh/patternfly-4/styles/tokens/

If these colors don't exist, maybe we can subtitute an existing pf-next color?

yes, I am planning to do the css cleanup

dgutride · 2019-01-22T21:14:49Z

This seems like a great idea; however, we are avoiding adding anything to react-core or the core packages (which include react-table) that do not have backing PatternFly patterns and documentation. Would it be possible to move this to a new @ patternfly / {name of packaqge} - extension package so we can consider including it into core PatternFly at a later date?

dmiller9911 · 2019-01-23T20:23:11Z

packages/patternfly-4/react-table/src/components/Table/Body.js

@@ -2,13 +2,14 @@ import React from 'react';
 import { Body } from 'reactabular-table';
 import PropTypes from 'prop-types';
 import { TableContext } from './Table';
+import '../../../internal/closestPolyfill';


This is a sideEffect and will break webpack 4 (well not be imported in production)., It is also generally a bad idea to pollute prototypes for the user. This should be an opt-in pollyfill by the consumer. We can still export it and if they want/need it they can use it. Also since by default PF4 is excluding IE support (a workaround is coming for consumers needing it) the closest polyfill is not needed.

If we do for sure want to keep this in the lib it would be better to do something like:

function closest(element, selector) { if (typeof element.closest === 'function') { return element.closest(selector); } if (document.documentElement.contains(el)) { while (el && el.nodeType === 1) { if (el.matches(selector)) return el; el = el.parentElement || el.parentNode; } } return null; }

Good to know. I just assumed it would be the same as PF3 so I copied it from there. Removed

dmiller9911 · 2019-01-23T20:23:49Z

packages/patternfly-4/react-table/internal/closestPolyfill.js

+export const matches =
+  Element.prototype.matches || Element.prototype.msMatchesSelector || Element.prototype.webkitMatchesSelector;
+
+if (!Element.prototype.matches) {


Not needed at all. This is supported by all major browsers including IE9+

dmiller9911 · 2019-01-23T20:24:21Z

packages/patternfly-4/react-table/internal/closestPolyfill.js

+  return null;
+}
+
+if (!Element.prototype.closest) {


See comment in packages/patternfly-4/react-table/src/components/Table/Body.js

dmiller9911 · 2019-01-23T20:28:28Z

packages/patternfly-4/react-table/src/components/Table/Body.js

+            onRowClick(event, row, props);
+            if (onEditCellChanged) {
+              // edit cell after rerender
+              setTimeout(onEditCellChanged, 0);


This seems kind of "hacky" with upcoming react releases it is possible that the rerender may not have actually taken place here. will that be ok? I can't tell from the code if that will cause issues or not.

In that case we would really have to contribute to reactabular and fix these issues. Yup it is hacky, but there are not many other solutions.

atiratree · 2019-02-01T14:45:15Z

This seems like a great idea; however, we are avoiding adding anything to react-core or the core packages (which include react-table) that do not have backing PatternFly patterns and documentation. Would it be possible to move this to a new @ patternfly / {name of packaqge} - extension package so we can consider including it into core PatternFly at a later date?

Sorry for late response because of the PTO.

I get the idea of putting bigger features to separate packages.
Nevertheless, I think inline editing is quite integral part of the table and it would be less complicated for the user to not use multiple packages. It is possible, but it would be quite cumbersome and not quite worth it in my opinion.

Btw, do you mean patternfly next patterns or https://www.patternfly.org/pattern-library/forms-and-controls/inline-edit/ ?

karelhala

Really nice extension! I like how you are passing different editors for different edits. That is really great approach. Few nitpicks, but overall good PR!

karelhala · 2019-02-02T09:38:16Z

packages/patternfly-4/react-table/src/components/Table/Body.js

-      onClick: (event) => onRowClick(event, row, props)
+      row,
+      rowProps: props,
+      onMouseDown: (event) => {


One note, can this function be moved outside of this component? So it would be easier to read the component when debugging?

karelhala · 2019-02-02T09:47:03Z

packages/patternfly-4/react-table/src/components/Table/RowWrapper.js

-    hidden={isExpanded !== undefined && !isExpanded}
-  />
-);
+const hasConfirmationButtons = (editConfirmationType, { isEditing, isTableEditing, isFirst, isLast }) => {


These function with switch can be converted to object, this approach is much more extensible.

const tableMapper = { [TableEditConfirmation.TABLE_TOP]: ({ isTableEditing, isFirst }) => isTableEditing && isFirst, [TableEditConfirmation.TABLE_BOTTOM]: ({ isTableEditing, isFirst }) => isTableEditing && isLast, [TableEditConfirmation.ROW]: ({ isEditing }) => isEditing, [TableEditConfirmation.NONE]: () => false }

You would then use it like

const hasConfirmationButtons = (editConfirmationType, props) => { tableMapper[editConfirmationType] && tableMapper[editConfirmationType](props); }

you are right, it looks much cleaner now

karelhala · 2019-02-02T09:47:22Z

packages/patternfly-4/react-table/src/components/Table/RowWrapper.js

+  }
+};
+
+export const areButtonsOnTop = (editConfirmationType, { isLast }) => {


Similiar approach as with the previous function.

karelhala · 2019-02-02T09:49:20Z

packages/patternfly-4/react-table/src/components/Table/RowWrapper.js

+  let editStyle;
+
+  if (isTableEditing) {
+    switch (editConfirmationType) {


Honestly I don't really like switches, they tend to be really complex, hard to read and even harder to extend. Please consider using object literal with functions.

karelhala · 2019-02-02T09:54:09Z

packages/patternfly-4/react-table/src/components/Table/RowWrapper.js

+    }
+    const { editConfirmationType, onEditConfirmed, onEditCanceled } = editConfig;
+
+    let confirmButtons;


No need to assign to variable if we are not gonna do anything with it and just return it. It's much cleaner to just return from if statement. When the if is not satisfied function will return undefined nevertheless.

I would rather remove it too, but I kept that because of eslint/prettier. I am not sure how do we stand on that?

karelhala · 2019-02-02T09:57:16Z

packages/patternfly-4/react-table/src/components/Table/Table.js

+};
+
+export const TableEditConfirmation = {
+  NONE: 'NONE', // TODO: do edit buttons make sense on the left?


This question is better to be asked any UX person @mcarrano any thoughts on this?

karelhala · 2019-02-02T09:58:26Z

packages/patternfly-4/react-table/src/components/Table/TableConfirmButtonsRow.js

+};
+
+// TODO: shouldComponentUpdate?
+// TableConfirmButtonsRow.shouldComponentUpdate = true;


As long as you are not trying to calculate re-renders for you we don't really need to use shouldComponentUpdate

karelhala · 2019-02-02T10:01:48Z

...patternfly-4/react-table/src/components/Table/utils/formatters/inlineEditFormatterFactory.js

+) => {
+  const { rowData, columnIndex } = additionalData;
+
+  let result;


No need to assign to result if we are only returning it. You can add return instead of assign to each if statements and it wil be the same.

you are right, thanks

karelhala · 2019-02-02T10:07:56Z

@suomiy what @dgutride is proposing is kinda understandable. We want to ship table component as pure as possible with only few enhancers and if consumers want to use inline edit they'd import @patternfly/react-table-inline-edit for instance. Inside of this package there would be few functions and components reponsible for edits. The code would still be part of this repository, it would just be moved into special folder and a bit of config is needed. If you want I can help you by moving it there or you can get inspired by @priley86 #1250 about virtualized scroll.

atiratree · 2019-02-04T11:19:22Z

@suomiy what @dgutride is proposing is kinda understandable. We want to ship table component as pure as possible with only few enhancers and if consumers want to use inline edit they'd import @patternfly/react-table-inline-edit for instance. Inside of this package there would be few functions and components reponsible for edits. The code would still be part of this repository, it would just be moved into special folder and a bit of config is needed. If you want I can help you by moving it there or you can get inspired by @priley86 #1250 about virtualized scroll.

Ok, I guess it makes sense to separate it. I will look into that.
Nevertheless I think there will still be some overlap of the code. I will discuss this with @priley86.

patternfly-build · 2019-02-04T11:28:39Z

PatternFly-React preview: https://1227-pr-patternfly-react-patternfly.surge.sh

karelhala · 2019-02-04T11:31:55Z

@suomiy if you need some special variables to be passed to core implementation that is fine, that code won't be used. But if we split this into another package we have really great way of importing custom packages and promote other teams to contribute and use some other packages (even written outside of PF world)

jschuler · 2019-03-14T17:32:01Z

I've removed the Do not merge label since it's in its own package now @dgutride
@priley86 @karelhala please review again :)

atiratree · 2019-03-21T19:40:39Z

Fixed TS and added support for rendering NO_CONFIRM_ROW type.

This PR is ready for final reviews (sibling #1520 was merged).

priley86 · 2019-03-22T13:12:33Z

great job on this @suomiy! I know you are eager to iterate on this extension downstream. I filed my follow-up issue just now in #1628. I am happy to see this merge when others are.

priley86 · 2019-03-22T15:11:18Z

getting these here locally now...can we fix them first?

@patternfly/react-docs: error There was a problem parsing component metadata for file: "components/InlineEdit/editableRowWrapper.js"
@patternfly/react-docs: 
@patternfly/react-docs:   TypeError: Property name must be an Identifier or a Literal
@patternfly/react-docs:   
@patternfly/react-docs:   - getPropertyName.js:36 getPropertyName

atiratree · 2019-03-22T18:48:54Z

getting these here locally now...can we fix them first?

fixed, seems like we can't have dynamic object keys (reactjs/react-docgen#317 (comment))

+ added a comment about this issue to the code as the constants are now hardcoded

packages/patternfly-4/react-inline-edit-extension/package.json

priley86

react-docs look good and tests pass locally. Please just update the patternfly dependency, but will go ahead and approve. nice work!

karelhala

Looks really good! Thank you for this PR!

atiratree · 2019-03-25T12:26:10Z

updated @patternfly/patternfly dependency

jschuler

Thanks @suomiy , great work on this!

atiratree commented Jan 22, 2019

View reviewed changes

karelhala self-requested a review January 22, 2019 13:38

priley86 reviewed Jan 22, 2019

View reviewed changes

dgutride added the Do Not Merge label Jan 22, 2019

priley86 mentioned this pull request Jan 22, 2019

fix(utils): export common utility functions and constants in react-core #1232

Merged

dmiller9911 reviewed Jan 23, 2019

View reviewed changes

atiratree force-pushed the pf4-table-inline-edit branch 2 times, most recently from ac5c96e to f8864c5 Compare February 1, 2019 17:16

karelhala reviewed Feb 2, 2019

View reviewed changes

atiratree force-pushed the pf4-table-inline-edit branch from f8864c5 to 18067b4 Compare February 4, 2019 11:02

tlabaj added the PF4 label Feb 4, 2019

atiratree force-pushed the pf4-table-inline-edit branch 4 times, most recently from 645f600 to e2ce121 Compare February 15, 2019 13:15

atiratree mentioned this pull request Feb 15, 2019

feat(table): add inline editing + buttons patternfly/patternfly#1428

Closed

atiratree force-pushed the pf4-table-inline-edit branch 3 times, most recently from 18a037b to 47b70e6 Compare February 15, 2019 14:57

jschuler removed the Do Not Merge label Mar 14, 2019

atiratree force-pushed the pf4-table-inline-edit branch 4 times, most recently from dd96fd9 to 1d8b7f2 Compare March 21, 2019 19:38

jschuler previously approved these changes Mar 22, 2019

View reviewed changes

feat(Table): support inline editing in PF4

d548402

atiratree dismissed jschuler’s stale review via 931e249 March 22, 2019 18:45

atiratree force-pushed the pf4-table-inline-edit branch from 1d8b7f2 to 931e249 Compare March 22, 2019 18:45

priley86 reviewed Mar 22, 2019

View reviewed changes

packages/patternfly-4/react-inline-edit-extension/package.json Outdated Show resolved Hide resolved

priley86 previously approved these changes Mar 22, 2019

View reviewed changes

karelhala self-requested a review March 25, 2019 08:11

karelhala previously approved these changes Mar 25, 2019

View reviewed changes

atiratree dismissed stale reviews from karelhala and priley86 via 21ca9d8 March 25, 2019 10:22

atiratree force-pushed the pf4-table-inline-edit branch from 931e249 to 21ca9d8 Compare March 25, 2019 10:22

feat(Table): support inline editing for collapsible rows in PF4

dfb18fd

atiratree force-pushed the pf4-table-inline-edit branch from 21ca9d8 to dfb18fd Compare March 25, 2019 12:09

karelhala approved these changes Mar 25, 2019

View reviewed changes

jschuler approved these changes Mar 25, 2019

View reviewed changes

jschuler merged commit b6eb553 into patternfly:master Mar 25, 2019

priley86 mentioned this pull request Apr 22, 2019

add bodyWrapper type def #1829

Merged

priley86 mentioned this pull request Jun 17, 2019

(Table): allow passing custom row, header and cell wrappers #1310

Closed

KKoukiou mentioned this pull request Oct 30, 2020

ListingTable onRowClick reacts to "mousedown", not "click" #5085

Closed

		@@ -0,0 +1,8 @@
		// TODO: better sharing util components between modules

feat(Table): support inline editing in PF4 #1227

feat(Table): support inline editing in PF4 #1227

Conversation

atiratree commented Jan 22, 2019 • 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

priley86 Jan 22, 2019 • 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

Choose a reason for hiding this comment

coveralls commented Jan 22, 2019 • edited Loading

Pull Request Test Coverage Report for Build 4563

💛 - Coveralls

priley86 commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgutride commented Jan 22, 2019 • 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

atiratree commented Feb 1, 2019

karelhala 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

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

karelhala commented Feb 2, 2019

atiratree commented Feb 4, 2019

patternfly-build commented Feb 4, 2019

karelhala commented Feb 4, 2019

jschuler commented Mar 14, 2019

atiratree commented Mar 21, 2019

priley86 commented Mar 22, 2019

priley86 commented Mar 22, 2019

atiratree commented Mar 22, 2019

priley86 left a comment

Choose a reason for hiding this comment

karelhala left a comment

Choose a reason for hiding this comment

atiratree commented Mar 25, 2019

jschuler left a comment

Choose a reason for hiding this comment

atiratree commented Jan 22, 2019 •

edited

Loading

priley86 Jan 22, 2019 •

edited

Loading

coveralls commented Jan 22, 2019 •

edited

Loading

dgutride commented Jan 22, 2019 •

edited

Loading