to allow uncontrolled components like TableTextInput

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';

thanks

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?

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?

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

possibility to select editing programatically

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

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

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

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

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.

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

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

Similiar approach as with the previous function.

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.

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?

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

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

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