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

🐾 Add a fast Create Plan page - step 1 #822

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

rszwajko
Copy link
Contributor

@rszwajko rszwajko commented Jan 12, 2024

Implements #801

Initial state

Screenshot from 2024-01-16 11-46-46

Plan name edited

Screenshot from 2024-01-16 11-46-53

@yaacov
Copy link
Member

yaacov commented Jan 14, 2024

nice 👍

added comment in #801 (comment) for separation of #801 into steps

@yaacov
Copy link
Member

yaacov commented Jan 15, 2024

@rszwajko , can you update the title and description to indicate this is a PR for the first step, "Add fast migration page" may indicate this PR actually adds the complete fast migration form ?

@rszwajko rszwajko changed the title Add fast migration page Add a fast Create Plan page - step 1 Jan 15, 2024
@rszwajko
Copy link
Contributor Author

@yaacov
Changes in the recent force push:

  1. transfer VM objects via context (previously only the IDs)
  2. treat context usage as internal implementation and hide it behind useCreateVmMigrationData() hook
  3. implement part of the spec provided in this comment
    • [name of source provider]-[short hash]
    • source provider as a resource link with label

Parts not implemented (yet):

  1. switch from form-based create page to template-based (similar to edit/details pages) - this requires some more though: first details-like page assumes that the resource is already valid and just maintains this invariant. Second problem is with cross-field dependencies - some user action will invalidate multiple fields i.e. changing the target provider invalidates also the namespace and the mappings.
  2. displaying the VMs as a table - this may break the flow we can try to teach to the user - if user can see the same table as on previous screen then why to have a second screen in the first place? why not allow de-selecting and selecting the VMs?

@rszwajko rszwajko marked this pull request as ready for review January 15, 2024 17:59
@yaacov
Copy link
Member

yaacov commented Jan 15, 2024

switch from form-based create page to template-based (similar to edit/details pages

inmplementing the "create and edit" button is the last step, no need to do it now

this requires some more though: first details-like page assumes that the resource is already valid and just maintains this invariant.

yes, it's "create" the plan is valid and created, "and edit" you go to the details/edit page

Second problem is with cross-field dependencies - some user action will invalidate multiple fields i.e. changing the target provider invalidates also the namespace and the mappings.

why will you want to change the target, that is not part of the spec

@yaacov
Copy link
Member

yaacov commented Jan 15, 2024

displaying the VMs as a table - this may break the flow we can try to teach to the user

that is why it need to be a "lateral" flow, a user will not open the vm list unless they need what to check somthing, "lateral" is somthing outside the regular flow - e.g. a modal or a side pane, the user see it check they didn't make a mistake in the last phase and continue.

if user can see the same table as on previous screen then why to have a second screen in the first place?

they will not open the vm list unless they wnat to check somethig, but if they do want to check, they will not need to leave the create form, the quicly see the list and then close the modal or side pane

why not allow de-selecting and selecting the VMs?

the source, and vm list are the unique featuers of this plan, you should not touch them, if you want to change them, cancel this plan and start a new one with other source and vm list

Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things to finish
a. remove description field (not in "fast create flow")
b. show only "[N] vms selected" (we can leave the side pane with the vm list to later step)
c. remove current target provider and target namespace because they will need hooks to get them, drop down implementation, and some logic to indicate to users that once you choose them you reset the mapings.
we can move the implementation to the next step

Changes:
1. transfer data from VM List screen to Create Plan page
   via console.context-provider
2. add immer library to keep data in the reducer immutable
3. add uuid library to generate random suffixes
4. use forms for the Create Plan page - similar to Create Provider

Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
Copy link

sonarcloud bot commented Jan 16, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@rszwajko
Copy link
Contributor Author

@yaacov
implemented most of your suggestions - the detail view for the VMs needs to wait for the next step (unless you want to postpone this one).

as for the discussion above:

this requires some more though: first details-like page assumes that the resource is already valid and just maintains this invariant.
yes, it's "create" the plan is valid and created, "and edit" you go to the details/edit page

just to be clear the plan displayed on the screen won't be valid in all cases

Second problem is with cross-field dependencies - some user action will invalidate multiple fields i.e. changing the target provider invalidates also the namespace and the mappings.
why will you want to change the target, that is not part of the spec

It's part of the spec. You explicitly requested an editable drop down.

regarding VM details representation:

that is why it need to be a "lateral" flow,

OK so displaying table in the standard view is not possible. We could display it in list view (see below). The view is automatic for small spaces.
image

@rszwajko rszwajko requested a review from yaacov January 16, 2024 11:00
Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yaacov yaacov merged commit 3bf74f1 into kubev2v:main Jan 16, 2024
7 checks passed
@yaacov yaacov changed the title Add a fast Create Plan page - step 1 🐾 Add a fast Create Plan page - step 1 Jan 17, 2024
@yaacov yaacov added the enhancement Categorizes issue or PR as related to a new feature. label Jan 17, 2024
@yaacov yaacov added this to the 2.6.0 milestone Jan 17, 2024
@yaacov yaacov added the plans label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Categorizes issue or PR as related to a new feature. plans
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants