First pass at updating the examples for the patternfly/react-table library

[riccardo-forina]

What:

I have personally struggled with patternfly/react-table's examples for a number of reasons:

• examples are provided as class-based components. This is ok, but some of us moved to function components (with hooks. Given that that seems to be the direction the React community is following, I think it makes sense to update the examples to the new style.
• heavy and confusing use of setState. I think setState in an example should be used only if the example requires it. But more importantly, for a developer that's still learning React it teaches a bad lesson. The React way should be to derive the state required for the rendering as much as possible, and avoid storing multiple copies of the same data using the state.
• arrays and objects are mutated in place. This is a bad practice in general, but with hooks it's also a pitfall since all hooks perform a shallow comparison between objects, which doesn't catch object mutations.

So I took a stab at updating the examples (well, some of them for the moment) to address the above points.

Finally, when updating the selectable rows example I got bit really hard by something that looked like a bug in the library but ended up being the right behavior. Using an immutable data structure and keeping track of the selected rows by storing the id in some state, this is what was happening:

1. initial render

Select	Name	Notes
[ ]	Foo
[ ]	Bar
[ ]	Baz

2. we select the Bar line

Select	Name	Notes
[ ]	Foo	this row will not be re-rendered
[x]	Bar	this row will be re-rendered because it's been selected
[ ]	Baz	this row will not be re-rendered

3. we select the Baz line. We expect Bar to stay select, but it gets deselected.

Select	Name	Notes
[ ]	Foo	this row will not be re-rendered
[ ]	Bar	this row will be re-rendered but with an unchecked state, because that was its state when Baz was rendered the first time
[x]	Baz	this row will be re-rendered because it's been selected

This was happening because the Foo's and Baz's onSelect callback was not being updated because the BodyRow:shouldComponentUpdate function correctly detected that nothing changed in the data, and skipped the render.
The solution was simple, the onSelect callback had to make use of useRef to track the latest snapshot of the selected rows.

I then realized that the logic for the selectable table wasn't trivial, especially for a developer new in the React ways, and also we were exposing a lot of logic to the end-user that I think we could abstract away. So I moved all the logic to a reusable hook called useSelectableRows that does all the above for the user, asking only for the rows and, optionally, a function to retrieve the row's key.

The example is now super short and I think we can save other devs some good amount of time because of this little hook.

I plan to do apply the same treatment to the remaining examples, but I'll probably not be able to do so for a few weeks. In the meantime, I'd like to collect your feedback about these changes. Are you all ok with it? Something that can be improved or changed?

Additional issues: kinda related #2358

[priley86]

Hey @riccardo-forina !

Really awesome points, and love this feedback! ❤️

A couple things come to mind for me personally:

• Completely agree we should start targeting the React Hooks API in future PF React components and examples. I believe this is the first such addition so a major kudus on this! ⭐️ I think this may spark a larger discussion about when to move away from class based components here... cc: @dgutride @dlabaj @jcaianirh

• Overall, I can see a lot of improvements that can be made to @patternfly/react-table if adopting Hooks. Using useContext Hooks for providing the current TableContext would be much cleaner as well down the road...

• Agree that using setState is non desirable and useState will be preferable once we adopt Hooks. Much of this is legacy and the way Reactabular showed that rows and cols were ultimately stateful (and dynamic) but it is better to think of them as props from a React perspective. Some consumers had these tied to Redux for providing table data and attributes, but I've noted a really good blog on how to move away from Redux and the old React state models like you propose here. I see a ton of value in moving towards this kind of API for React based consumption.

• Regarding BodyRow:shouldComponentUpdate and useRef - very good catch w/ this. I think useSelectableRows is a nice addition and exactly the kind of API feedback we are looking for now that the Reactabular source is "in-house". Happy to see these kind of improvements coming!

Overall, I don't have any issue moving the React Table docs here towards Hooks if others are OK w/ this effort. I think it would be a really nice addition to test alongside #2360 after that PR goes and set the stage nicely for future...

Will add a few inline comments and await any feedback from others!

[priley86]

nice! I think these comments certainly help understanding. That is the goal!

[seanforyou23]

The comments really do go a long way. Would you be willing to add a couple of similar style comments to some other bits like useSortableRows/useSelectableRows/sortLastCommit/useMemo?

[riccardo-forina]

Sure!

[priley86]

I am all for showing examples of custom CSS applications if they are useful and match PF standards. I don't know if we want a single "style customized" example or many, but I like this. "Cell Formatters" example is a very welcome addition. We could potentially have other examples of this coming in the future too (i.e. a table that has "favorited rows") cc: @patternfly/patternfly-core-ux

[mturley]

I agree that these examples might be useful, maybe a section of the docs page on "customizing styles" with a few examples would be good in the long term, but we could start with just one.

As far as using the --pf-global variables directly as a string here, @priley86 do you happen to know if there is a way to import these color variables as constants from @patternfly/react-styles somehow?

[seanforyou23]

Love the "favorited rows" idea as an example.

@mturley yep, we could do something like import { global_breakpoint_lg } from '@patternfly/react-tokens'; which basically results to

{
    "name": "--pf-global--breakpoint--lg";
    "value": "992px";
    "var": "var(--pf-global--breakpoint--lg)";
}

[riccardo-forina]

I'm not sure why the doc fails building. This is the error:
@patternfly/react-docs: WebpackError: ReferenceError: CheckIcon is not defined
But it's working fine in dev mode, and I believe the icon is there?

[riccardo-forina]

Today I focused my efforts making sure that sortable and selectable tables are easier to pull off. There is now a useSortableRows hook that allows a user to sort the rows automatically, providing the right sort function to the cell. There are a few common sorting functions already implemented and usable, but a custom one can be provided too (check the example with the cell with the emoji).

I also updated the Typescript typings a bit. Not sure how friction-free with the PR to migrate the lib to TS will be, but it helped in writing the hooks.

I doubt I'll be able to push any more updates to this for a few weeks, and CI doesn't pass for reasons I'm not sure I understand. So if you feel like closing this and have me reopen it again in the future I'll understand :D
Else if anyone wants to jump in and push PRs against my branch, that works too.

[mturley]

+1, this is a really cool direction for us to move in! I love the introduction of Hooks, that will be a great way to clean up some of the more convoluted patterns in the original reactabular source. Nice work @riccardo-forina !

I do think we should rebase/restart this after #2360 is merged, because there will be conflicts and because converting the whole of react-table to TypeScript before making changes like this to the API/behavior will make the history easier to track.

[patternfly-build]

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

[riccardo-forina]

Ok I finally managed to sync this back with upstream and find a way around the sortable API change I did introduce.

Are you ok with reviewing this now? I plan to follow up with other PRs for the remaining examples to avoid making this too big.

[seanforyou23]

Looking good! I'm going to continue testing these changes out but on PTO tomorrow then the weekend so wanted to share the brief feedback I have from the first pass. Overall it's looking awesome though, nice work!

[seanforyou23]

This example isn't working for me, haven't figured out a fix yet but will keep looking. Any ideas? Other examples seem to be working fine FWIW.

[riccardo-forina]

I see a Table.d.ts referenced there, but there is no such thing anymore. Wondering, did you run a full build?

[seanforyou23]

Good call, not sure why I'm still seeing the old files, totally did a full rebuild, must be the caches. Give me a bit to re-test this and I'll update my comments - for now please ignore them!

[seanforyou23]

The comments really do go a long way. Would you be willing to add a couple of similar style comments to some other bits like useSortableRows/useSelectableRows/sortLastCommit/useMemo?

[seanforyou23]

One more thing, I started with the code from the CellFormatters example as a base, then, I attempted to incorporate some behavior from the "compact expandable table" example but hung and realized that now some examples use a basic array instead of array of objects representing the rows. I wonder if it would be too much trouble to align this one thing, that way the demos go together easier like legos?

I then think about this sort of related comment Please do not use rowKey as row index for more complex tables. Rather use some kind of identifier like ID passed with each row. and wonder if we could use this as an opportunity to make exactly that change (since we're here in the spirit of DX) in the demo code instead of suggesting it to each user to do on their own? Especially for the simple case. The more we can set users up out of the box the better and these changes might help put people onto the fast track. Thoughts?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[mturley]

@riccardo-forina @seanforyou23 this got marked stale, do we think it's worth rebasing and trying to get it moving again or should we close it?

[riccardo-forina]

I know that the table package is being actively developed, unfortunately I don't have much time to update this. I'm ok for closing it unless @seanforyou23 is of a different view.

[seanforyou23]

Thanks to everyone for being patient while this one sat on the back burner. We've finally worked through several critical type issues that we needed to have in place before we could really think about making moves on things mentioned in this PR. Here's where I think we're at..

We want to embrace the idea of keeping the table and example implementations current, and also balance that with continuing the support the existing implementations which may be favorable to some. We have learned from this PR and implemented a hook based approach in our beginner katacoda tutorial, it was easier to use this new pattern here because there was no pre-existing code to combat with. I think it's been well received and hope that we can continue to add more of this style tutorial.

arrays and objects are mutated in place

This ^ is a critical point in my opinion. When I follow the examples from our docs and convert them into function components, some of the things simply do not work as expected. For example, these lines do not work for me in a function component based approach, and I think it's for the reason you mention, that we are mutating objects in place.

const { rows } = this.state;
rows[rowKey].isOpen = isOpen;
// then set state with `rows`

The fix was simple, just create a new array based off of the rows state property, and modify that instead, then set state with that. I think at the very least we should make this type of change to our class-based component examples. This goes to show how function components can illuminate problems in the code we don't realize even exist.

There have been some enhancements made to the selectable rows doc, and I think further enhancements would be welcome. useSelectableRows looks really awesome and I love the idea of offsetting complex logic like this so each user doesn't have to reinvent it. The truth is, though, that we're still building our competency as a team on the hooks-based approach, and we need to make sure we can support these types of design patterns as a team before we go all-in on them in the docs. For the sake of incremental progress, I say let's make the action item to fix these cases of mutating objects in place so that at the very least, those who wish to use hooks/function components can more easily copy code from our examples without immediately running into problems. I don't think we can have too many examples/demos, so instead of converting the existing examples to hooks, can we instead add new examples/tutorials that illustrate these concepts so that all people have bases covered, regardless of their preferred pattern?

That's the best and solution I can think of at the moment. In that case, we can close this PR and resurrect it as time allows as an addition to the docs instead of a refactor. Does that cover all our bases? I will send a PR to fix the in-place mutations to soften the problem in the mean time.

Let me know what you think, sorry for the wall, just wanna make sure all the context was captured for anyone interested to see. @riccardo-forina @mturley lemme know what you think.

[riccardo-forina]

Thanks for this! I’m 👍 on everything, examples can (maybe should) be showing how to do things both with classes and function components. It makes sense to find a way to do it.

[mturley]

Totally agree on all points. Especially the in-place mutations.. I don't know if that's a practice we inherited from Reactabular, but it for sure needs to go. To be honest, ever since we decided to pull Reactabular's source in house and convert it to TypeScript, I started asking myself why that became necessary and why the original Reactabular devs stopped maintaining it. It's possible we will need to move farther away from their original implementation ideas to make sure ours can keep up.

I think it makes sense to modernize what we can when we can, while coming to speed with things like hooks, and honoring the original implementation for those who already use it. Thanks for looking at this @seanforyou23 !