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

Enhance PVs step of plan wizard: switch to PF table, fix useEffect logic, add types, other UX cleanup #776

Merged
merged 10 commits into from Apr 2, 2020

Conversation

@mturley
Copy link
Collaborator

mturley commented Mar 26, 2020

Closes #586

  • Lifts non-table-presentation-related logic out of VolumesTable into VolumesForm.
  • Moves table header text down into VolumesTable so it doesn't appear during discovery/polling.
  • Fixes an issue with the useEffect logic where getPVResourcesRequest was being called four times instead of once, causing ~500ms / 2KB of extra network requests (see useEffect note below)
  • Converts the PVs table to a PatternFly Table
  • Converts the react-select components to PatternFly Select
  • Abstracts out a new reusable React hook: usePaginationState (from code I wrote for NameSpacesTable pagination in #749)
  • Adds TypeScript prop type interfaces to VolumesTable and VolumesForm components, by using Pick<> to share types with WizardContainer, where I added some more specific types.
  • Changes navigation and table column headers to sentence-case capitalization.

Screens

Before:

  • Non-PF react-table and react-select
  • Header text appears before table is loaded
  • Title-case capitalization on nav and headers

Screenshot 2020-04-01 15 54 58

Screenshot 2020-04-01 15 55 21

After

Screenshot 2020-04-01 15 47 25

Screenshot 2020-04-01 15 47 35

Note about useEffect

Because of how useEffect's dependency array argument works, it's easy to have useEffect logic called more often than necessary. It's important to specify very specific things in that array so that the effect will only run again if that specific thing changes.

In this case, the useEffect that waits for discovered PVs to be present and then calls getPVResourcesRequest was using [currentPlan, isPollingStatus] as its dependencies, which means it was running again every time the component re-rendered (since currentPlan comes from redux state, and each state change creates a new immutable currentPlan, it is always changing). What we really needed was for that logic to only run when currentPlan.spec.persistentVolumes goes from empty to non-empty, so the real dependency is just the length of that array.

On entering the wizard step, network traffic before 2ecb64f:
Screenshot 2020-03-25 19 49 32-bad

Network traffic after 2ecb64f:
Screenshot 2020-03-25 19 53 10-good

@mturley mturley changed the title [WIP] Enhance PVs step of plan wizard: switch to PVs table, fix useEffect logic, other UX cleanup [WIP] Enhance PVs step of plan wizard: switch to PF table, fix useEffect logic, other UX cleanup Mar 26, 2020
@mturley mturley force-pushed the mturley:586-plan-wizard-PVs-enhancements branch 2 times, most recently from 5db26a2 to 6a2eeb5 Mar 30, 2020
@mturley mturley force-pushed the mturley:586-plan-wizard-PVs-enhancements branch from 6a2eeb5 to d74ac6f Mar 30, 2020
@mturley mturley force-pushed the mturley:586-plan-wizard-PVs-enhancements branch from 303e176 to dec93f3 Mar 31, 2020
@mturley mturley changed the title [WIP] Enhance PVs step of plan wizard: switch to PF table, fix useEffect logic, other UX cleanup [WIP] Enhance PVs step of plan wizard: switch to PF table, add sorting/filtering, fix useEffect logic, add types, other UX cleanup Apr 1, 2020
@mturley mturley changed the title [WIP] Enhance PVs step of plan wizard: switch to PF table, add sorting/filtering, fix useEffect logic, add types, other UX cleanup [WIP] Enhance PVs step of plan wizard: switch to PF table, fix useEffect logic, add types, other UX cleanup Apr 1, 2020
@mturley mturley changed the title [WIP] Enhance PVs step of plan wizard: switch to PF table, fix useEffect logic, add types, other UX cleanup Enhance PVs step of plan wizard: switch to PF table, fix useEffect logic, add types, other UX cleanup Apr 1, 2020
@mturley mturley marked this pull request as ready for review Apr 1, 2020
@mturley mturley requested a review from konveyor/migration-engineering-ui-committers as a code owner Apr 1, 2020
Copy link
Collaborator

vconzola left a comment

Hey @mturley, this looks great. One UI nit... please repeat the wizard step title at the top of the page per the mockups. This is a PF design guideline.

@mturley

This comment has been minimized.

Copy link
Collaborator Author

mturley commented Apr 1, 2020

@vconzola you got it. Thanks for catching that.

@mturley

This comment has been minimized.

Copy link
Collaborator Author

mturley commented Apr 1, 2020

Actually @vconzola the wizard step title is missing from all steps of the wizard, so I think if it's ok with you guys I'm going to do that in a separate PR so this one stays relatively localized to this step. Broken out into a new issue: #795

}
}
// TODO do we really need to generate these meta-objects and keep them in formik?
// currentPlan.spec.persistentVolumes is in global state, so we could just

This comment has been minimized.

Copy link
@ibolton336

ibolton336 Apr 1, 2020

Contributor

+1 Sounds like a good idea to refactor to use currentPlan as the source of truth

}
setFieldValue('persistentVolumes', mappedPVs);
}
}, [discoveredPersistentVolumes.length]); // Only re-run the effect if fetching value changes

This comment has been minimized.

Copy link
@ibolton336

ibolton336 Apr 1, 2020

Contributor

+1 Nice job with the useEffect enhancements. Originally we were using useEffect to filter out all of the polling updates in the background causing rerenders. Glad this is getting some attention! Looks much better.

This comment has been minimized.

Copy link
@mturley

mturley Apr 1, 2020

Author Collaborator

Ah that makes sense. Unnecessary re-renders are still a thing I never know how to handle correctly.

getPVResourcesRequest: () => void;
getPVResourcesRequest: (
persistentVolumes: IPlanPersistentVolume[],
sourceClusterName: IFormValues['sourceCluster']

This comment has been minimized.

Copy link
@ibolton336

ibolton336 Apr 1, 2020

Contributor

image

^^^ so satisfying! Nice job building out the typescript support here.

// TODO add other properties used by components in this wizard, possibly move this somewhere more common
// Maybe in the future we can somehow generate types like these from the CRDs in mig-controller?

export interface IPlanPersistentVolume {

This comment has been minimized.

Copy link
@ibolton336

ibolton336 Apr 1, 2020

Contributor

+1 i am on board with the types.ts precedent for adding types to components going forward for now.

@mturley mturley dismissed vconzola’s stale review Apr 1, 2020

Moved feedback to new issue #795

</Bullseye>
const rows = persistentVolumes.map((pv, pvIndex) => {
const matchingPVResource = pvResourceList.find(pvResource => pvResource.name === pv.name);
const migrationTypeOptions = pv.supportedActions.map(

This comment has been minimized.

Copy link
@ibolton336

ibolton336 Apr 1, 2020

Contributor

seeing the app blow up here in some cases. I think we need a guard if supportedActions is undefined.

image

This comment has been minimized.

Copy link
@ibolton336

This comment has been minimized.

Copy link
@ibolton336

ibolton336 Apr 1, 2020

Contributor

Steps to reproduce: navigate back after selecting namespaces, select a new namespace & deselect old namespace, try to navigate forward and run pv discovery.

This comment has been minimized.

Copy link
@mturley

mturley Apr 1, 2020

Author Collaborator

Ah interesting... I wonder why you're getting the original PV from redux there and not the meta-PV-thingy defined here: https://github.com/konveyor/mig-ui/pull/776/files#diff-342db03f4d82174c47390c429f6aa190R77-R85 ... I'll take a look.

This comment has been minimized.

Copy link
@mturley

mturley Apr 1, 2020

Author Collaborator

It's strange that the same error wasn't being caused by this: https://github.com/konveyor/mig-ui/pull/776/files#diff-8d2e158fee7fbe8447115eb6c8a9725eL260-L264

This comment has been minimized.

Copy link
@mturley

mturley Apr 1, 2020

Author Collaborator

Wrote up an issue about this and the refactor I reference in a few of these comments: #797

@mturley mturley force-pushed the mturley:586-plan-wizard-PVs-enhancements branch 3 times, most recently from 0e853e9 to ad014dd Apr 1, 2020
@mturley mturley force-pushed the mturley:586-plan-wizard-PVs-enhancements branch from 68af7fd to 0c9e396 Apr 2, 2020
@mturley mturley requested a review from ibolton336 Apr 2, 2020
@ibolton336 ibolton336 merged commit 3109609 into konveyor:master Apr 2, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mturley mturley deleted the mturley:586-plan-wizard-PVs-enhancements branch Apr 2, 2020
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.

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