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

Clean up Select components and related state in Resources step of plan wizard #738

Merged
merged 7 commits into from Mar 12, 2020

Conversation

@mturley
Copy link
Collaborator

mturley commented Mar 11, 2020

This PR implements everything for #585 except the table changes.

  • I replaced the react-select component with patternfly's Select via a thin wrapper called SimpleSelect that I added to avoid repeating boilerplate for simple string selections.

    • react-select is still present in a few other places, but they are all covered by other issues so I'll address them individually. (#586, #587, #663, #736)
  • I replaced the default "Select..." placeholder text with "Select source...", "Select repository...", and "Select target..." per @vconzola's comment: #585 (comment)

  • The { label, value } objects used for options in the previous Select component are now unnecessary, and while Select does support complex values like this, they were adding nothing here so I simplified things by switching to plain string values for the select options.

  • The selected value of each Select was being stored both in React useState and up the tree in Formik state. I removed the local state and just used the Formik values as the source of truth so we don't have to keep redundant state synced up here.

  • The fieldId prop of FormGroup was used incorrectly: in order for it to associate labels to the right fields for screen-reader users, there must be an id prop on the actual field itself matching the fieldId.

  • Errors were rendered with a special <div id="feedback"> instead of the helperTextInvalid prop of FormGroup. (see #691 and #698)

  • When no valid source clusters are found, an option was rendered in the source clusters menu with the value "N/A". Selecting this value caused a JS error in the change handler:

    Uncaught TypeError: Cannot read property 'MigCluster' of undefined
        at Object.handleSourceChange [as onChange] (ResourceSelectForm.tsx?e280:144)
    

    To fix this error, I check that the matching cluster/storage exists before trying to reference its properties.

@mturley mturley requested a review from konveyor/migration-engineering-ui-committers as a code owner Mar 11, 2020
Copy link
Contributor

ibolton336 left a comment

Nicely done. Good call moving the values to Formik & extracting the values to strings. Looks way cleaner & easy to follow. Thanks @mturley - Tested in prod & looks all good.

@ibolton336 ibolton336 merged commit 6da0581 into konveyor:master Mar 12, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mturley mturley deleted the mturley:585-plan-wizard-step-2 branch Mar 12, 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.

None yet

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