feat(table): a second table tutorial #122
Conversation
There was a problem hiding this comment.
After each substep, there is a snippet of code like "Your component implementation should currently look something like the following". I kind of like that, but not sure it's necessary, considering some of these snippets are quite large. I'm thinking if we want this, it would be best to show a code snippet once at the very end of the step, perhaps after with the snapshot (like on step 8), instead of after each substep?
Step 2
4) Now, locate the line <Table caption="Patternfly React Table" cells={[]} rows={[]}> and replace the references to empty arrays (cells={[]} rows={[]}) with the row/column definitions you just created.
Please add a copy to clipboard control here.
Step3
2) For each of the column headers in the array, replace the string with an object and set the object’s title property to the string value you had previously.
Please add a copy to clipboard control here.
4) For each of these three row definitions, replace the entire string array with an object and set the object's cells property to the string array you had previously.
Please add a copy to clipboard control here.
5) For each item in each of your rows cells array, replace the string value with an object whose title property is the string you had previously.
We already modified the columns in substep 2. Thinking we should just show the modified rows here, since we're not changing columns at this point.
Thinking the previous values could be slightly different than these steps? Considering the substep values are all the same, it's easy to get confused where you left off.
Step 4
2) Add a variant prop to the Table component, and set its value to "compact".
Please add a copy to clipboard control here.
Should we skip the "Your Table definition should now look like the following" code?
Step 5
3) Navigate back to the src folder and open src/App.js and import your new row/column definitions. Place the import statement just below the last import at the top of the file.
Please add a copy to clipboard control here.
Step 6
1) Import Pagination component from react-core right after your imports from @patternfly/react-table.
2) Render the Pagination component just above the <Table> component, as a sibling in the component tree.
3) Set the itemCount prop of the Pagination component to the total number of rows in your dataset.
Please add a copy to clipboard control to each step above.
Should we skip the "Your component implementation should look like the following"?
Step 7
1) In your src/App.js file create two state properties within your component, numPerPage and currentPage, setting their values to 2 and 1, respectivly. Place this code inside of the App component, just below the line that starts with const App = () => {.
2) Set the perPage prop of the Pagination component to the numPerPage state property you just created.
3) Similarly, set the page prop of the Pagination component to the currentPage state property.
Please add a copy to clipboard control to each step above.
Can probably skip the "Your Pagination component should look like the code snippet below", since we're adding perPageOptions the pagination again in substep
6) Set the onPerPageSelect prop of the Pagination component to the function "handlePerPageSelect" you just created.
Please add a copy to clipboard control here.
Should we skip the "Your component implementation should currently look something like the following" ?
Step 8
Please add a copy to clipboard control to each step.
5) Add a useEffect hook inside your component that we'll teach to only run when certain properties change. This effects hook will ultimately ensure numPerPage and rows stay in sync. Inside this effect, make a call to handlePerPageSelect, pass null for the first param (event) and pass numPerPage as the second param. Place this code just above the return statement in your App component.
It's not clear where this code should be added.
Should we show a more basic way of using pagination Vs hooks? I don't feel strongly about using them, but it could be considered a more advanced topic?
In step 9, we show a basic function and hooks.
6) Locate the empty dependency array for the useEffect hook you just created, it's the second parameter ([]) that was passed to useEffect. This second parameter to useEffect represents an array of the function's dependencies.
There is nothing to do in this step, except locate the empty dependencies?
7) Specify numPerPage and handlePerPageSelect as effect dependencies by listing them in the dependency array from the previous step. Replace the empty array with the following;
It's not clear where this code should be added. Thinking steps 5, 6, and 7 should be combined.
Step 9
3) Now convert handlePerPageSelect to a callback hook by passing the function signature as the first parameter to React.useCallback(), and specify an array of dependencies for this callback hook like you did earlier for handlePerPageSelect. Omitting the specifics, it should generally take the form of: const handlePerPageSelect = React.useCallback(FN, [DEPS]);, where FN is your callback function and DEPS represents an array of any state properties or other callback functions.
Is there anything to do in step 3? It appears that the work to be done is in the next step?
What is the reason for changing the existing handlePerPageSelect to a callback hook? Should we bother showing steps 1 and 2 if we're just going to use hooks?
Should we just show the basic way of using pagination (i.e., without the hook)?
|
Thanks @dlabrecq!! I have a confession to make - I was using my single katacoda profile url to showcase some updates I made to the first tutorial, and didn't update my master branch to point back to the second table tutorial as described after that work was merged. It's kind of a downside to the current development flow, I don't know of a way to have different urls for different branches of work. I should probably just have one katacoda PR open at a time so this doesn't happen again. Sorry! The feedback is still relevant for the first tutorial, though, so we can go ahead and discuss it here if that works. For the clipboard copy additions, I initially added them to every code snippet as you suggested, but we received further feedback that it might be good to remove them in some cases. See this issue.
Right, I agree they are quite large and it seems like a lot at the moment. It seems inevitable that some steps will require additional information like this, and my previous approach was to describe the steps in words with a single snippet at the end but got plenty of feedback about needing more information along the way, so there doesn't seem to be a blanket rule that works across the board. I need to think on this some more, will try some things when I send a follow-up PR for intro-table with Pagination enhancements and ping you in it there if that's ok.
Wasn't quite sure what you meant by this, can you explain a little more?
We definitely can, I found it helpful to have the code-level confirmation after each step, it makes for a slightly longer read but leaves less to be confused about. If you think it's too much then we can definitely remove it.
Definitely shooting for the most basic way of implementing pagination, hooks seem to be the best way of doing this to me as it's just functions. I'm open to ideas, feel free to suggest anything you think would help.
I think it's ok, some people had trouble understanding this so I broke it down into more substeps so that each task can be completed individually. It's kind of in line with this suggestion.
Yea step 3 was supposed to be the work item, and step 4 was guiding the user through verifying the change. That's not really how we've been doing verifications now that I think about it. Will adjust.
Converting these functions to callback hooks is required to ensure they pass lint rules (which are there to ensure your code runs as expected within react component lifecycles). Without this step, if the user takes this code example and plugs it directly into codesandbox or patternfly seed project they will immediately be met with errors. That said, we absolutely can remove them if we think it's too much for the intro tutorial. I'd rather we decide to remove it after having seen what it "cost" us rather than not adding this step in the first place. It's a good talking point though, curious what others think. It's sort of a consideration to think about across all the react tutorials.
We can. I'm of the mindset that pushing forward with a hooks-based approach is simpler and better, even though it's a newer paradigm for patternfly codebase. Not dead set on it, but it was a fairly deliberate decision to write this tutorial based on hooks so I'd like to get some others to weigh in before re-writing it to use traditional classes. FWIW I believe the pagination enhancements I mentioned earlier will help simplify this enough that the steps related to working with hooks will be much less. If you're ok with it, I'd like to get that enhancement in place and have a look before we decide to dump the hooks based approach altogether. Does that sound like an OK approach? The rest seems straight forward, will make these updates with the rest of the work. |
|
Oops. Ok, let me know when the URL is pointing to the correct tutorial and I'll review again. |
|
Will do, I'll actually just close this PR for now since intro-table has more feedback to address and needs those pagination enhancement additions - will revive it once we've sorted the remaining intro-table issues and know how to proceed with the steps that utilize react dev tools, which is being discussed in #139 |
This PR adds a second tutorial (Customizing PatternFly React Table) for the table pathway. It covers adding classes and data attributes to table rows, columns, and cells. It also covers setting the key prop in various ways.
I haven't added any validation, but plan to in a follow-up PR if that's ok.
I couldn't figure out how to use the regular react dev tools browser extension to debug/verify the react key customizations in the preview area generated by katacoda. There is this package which we could possibly use as an in-app debugger. What do others think? Is this necessary to get the tutorial merged or could we add support for that in a follow-up PR?
https://katacoda.com/seanforyou23