Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace react-table with PF Table in Resources step of plan wizard #749

Merged
merged 8 commits into from Mar 19, 2020

Conversation

@mturley
Copy link
Collaborator

mturley commented Mar 13, 2020

Closes #585.

This replaces the react-table which was in place in this wizard step with PatternFly Table and Pagination components. In the process, I removed local state and adopted Formik's state as the source of truth for which rows are selected, similarly to #738. This made all of the useEffect logic in this file unnecessary.

Screenshot 2020-03-13 19 21 07

While implementing this, I got stuck for a while on an issue with the scope accessible from the onSelect function being out-of-date. I think this is a bug in the PF Table component, and I plan to reproduce it in a CodeSandbox and open an issue in PatternFly for that next week. For the time being, I worked around it by passing a reference to current selection state into each row object, and referencing that rowData in onSelect instead of directly referencing the component's main function scope.

@mturley mturley requested a review from konveyor/migration-engineering-ui-committers as a code owner Mar 13, 2020
@mturley mturley added the UXD label Mar 13, 2020
@mturley mturley force-pushed the mturley:585-plan-wizard-namespace-table branch from 8914bd7 to 7ef979f Mar 13, 2020
const rows = sourceClusterNamespaces.map(namespace => ({
cells: [namespace.name, namespace.podCount, namespace.pvcCount, namespace.serviceCount],
selected: values.selectedNamespaces.includes(namespace.name),
meta: { selectedNamespaces: values.selectedNamespaces }, // See comments on onSelect
}));
Comment on lines +41 to +46

This comment has been minimized.

Copy link
@ibolton336

ibolton336 Mar 14, 2020

Contributor

🏆

@eriknelson

This comment has been minimized.

Copy link
Member

eriknelson commented Mar 14, 2020

@vconzola this looking good to you?

@eriknelson

This comment has been minimized.

Copy link
Member

eriknelson commented Mar 14, 2020

Assumed we wanted to get #745 merged before this one, @mturley let me know if this needs any changes after a rebase, otherwise I think we're good to review and merge this 👍

Copy link
Member

eriknelson left a comment

Built and tested this rebased on master and it's looking solid! I did notice one issue:
image

Looks like hitting that dropdown produces an overflow scroll? I don't know if there's much that we can do about that.

@mturley

This comment has been minimized.

Copy link
Collaborator Author

mturley commented Mar 16, 2020

@eriknelson since the whole pane can scroll if the table is tall enough, I don't think we can avoid that. Allowing the popover to overflow would allow the whole table to overflow into the footer with the buttons.

I do know @vconzola wanted to move the table to its own wizard step to give it some more space, which would help with this.

@mturley

This comment has been minimized.

Copy link
Collaborator Author

mturley commented Mar 16, 2020

Actually, scratch that! We can't make the dropdown pop out of the scroll container, but we can make it "drop up" instead. I'll make that change for the pagination at the bottom of the table.

@mturley mturley force-pushed the mturley:585-plan-wizard-namespace-table branch from c57cc89 to 181d5c6 Mar 16, 2020
@mturley

This comment has been minimized.

Copy link
Collaborator Author

mturley commented Mar 16, 2020

Made that change and rebased on master. How's this @eriknelson ?

Screenshot 2020-03-16 11 56 40

@vconzola

This comment has been minimized.

Copy link
Collaborator

vconzola commented Mar 17, 2020

@mturley This looks good to me. One question...when the number of namespaces gets large enough to require scrolling, will the table scroll or the page scroll?

@mturley

This comment has been minimized.

Copy link
Collaborator Author

mturley commented Mar 17, 2020

@vconzola the whole body of the wizard step will scroll (not including the footer with the buttons).

Copy link
Contributor

ibolton336 left a comment

LGTM

@eriknelson

This comment has been minimized.

Copy link
Member

eriknelson commented Mar 18, 2020

@vconzola Are we okay with that scrolling behavior? Under most circumstances, this is giong to be the behavior given an average namespace count.

@mturley

This comment has been minimized.

Copy link
Collaborator Author

mturley commented Mar 18, 2020

@vconzola here's how it looks (including mobile responsiveness):
cV3VjSQgVo

@eriknelson

This comment has been minimized.

Copy link
Member

eriknelson commented Mar 18, 2020

@mturley

This comment has been minimized.

Copy link
Collaborator Author

mturley commented Mar 18, 2020

Oh yeah sorry for the confusion there @eriknelson, the table is paged (defaults to 10 namespaces per page, can be changed up to 100 per page). It's just that the current page of the table is part of the scroll container that also includes the dropdowns at the top. So you'll still have to scroll to get to the bottom of the current page.

@vconzola

This comment has been minimized.

Copy link
Collaborator

vconzola commented Mar 18, 2020

Thanks @mturley. This looks good.

@eriknelson eriknelson merged commit 3ff41a4 into konveyor:master Mar 19, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.