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.

Sign up for GitHub

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

Jump to bottom

Migrate Create VM Wizard to PF4 #1549

Merged
merged 6 commits into from
Jun 7, 2022
Merged

Migrate Create VM Wizard to PF4 #1549

merged 6 commits into from
Jun 7, 2022

Conversation

rszwajko
Copy link
Member

@rszwajko rszwajko commented Dec 23, 2021 •
edited
Loading

Depends on #1543

Final part of the PF3 -> PF4 migration. The only remaining PF3 elements are:

  1. react-console package - details in aa1e227
  2. VmUserMessages because PF4 replecement Notification Drawer is still beta (missing features need to be re-implemented) FIX IN Migrate masthead to PF4 and clean-up PF3 CSS  #1584
  3. PF3 styles - they are indirectly used across the product so a clean-up action is required FIX IN Migrate masthead to PF4 and clean-up PF3 CSS  #1584

@rszwajko rszwajko requested review from sjd78 and sgratch December 23, 2021 15:15
@rszwajko
Copy link
Member Author

Basic step

image

NICs

image

image

image

image

Disks

image

image

Review step

image

Finished

image

image

@ovirt-infra
Copy link

All tests passed

@rszwajko rszwajko mentioned this pull request Feb 1, 2022
Merged
@ovirt-infra
Copy link

All tests passed

@sjd78 sjd78 added the PF4 label Feb 25, 2022
@rszwajko rszwajko mentioned this pull request Mar 3, 2022
Merged
@ovirt-infra
Copy link

All tests passed

@rszwajko rszwajko mentioned this pull request Apr 13, 2022
Merged
@rszwajko rszwajko changed the title [WIP] Migrate Create VM Wizard to PF4 Migrate Create VM Wizard to PF4 Apr 13, 2022
@ovirt-infra
Copy link

All tests passed

@ovirt-infra
Copy link

All tests passed

@ovirt-infra
Copy link

All tests passed

@ovirt-infra
Copy link

All tests passed

@ovirt-infra
Copy link

All tests passed

@sjd78
Copy link
Member

sjd78 commented Jun 3, 2022

L&F / interaction issues (based on testing via #1594)

Basic Settings Step

  • I can change the Memory before selection cluster/source/template or iso/OS but I can't change the total CPUs. They should both be read only if other data needs to be selected first (e.g. to get the max counts per cluster level).

  • The Cluster shows as in error if one isn't selected but only if a name is entered. Not sure if "invalid" is the best choice, but it is reasonable. Maybe just start the form with an error on the name and go from there? No need to rehash the "direct the use through the form" debate we had a few years ago.

  • The Cloud-init / sysprep section title seems crowded up against the enable checkbox. Some top and bottom margins like in the PF3 version would look better.

  • The ExpandableSection for the CPU topology looks good.

  • Would be nice to had a slightly taller content area for the form to avoid scrolling, but it's manageable.

  • As noted in the VM Details Details card, the Operation System dropdown needs to scroll. It is painful to have to scroll the content area instead of the select box. Same thing would probably happen with a lot of clusters or ISOs or Templates. Seems like it would be a generic select box issue.

Networking Step

  • It all looks and works great!

  • Maybe the only thing would be static column widths, as the widths tend to jump around between view, edit, save.

Storage Step

  • The table look and editing looks good.

  • Picking templates with disks looks good.

  • I did NOT test the various permutations of thin/preallocated against the optimized for type or base templates or creation that requires selecting a new target storage domain.

Review Step

  • I miss the "Review and confirm setting" heading.

  • The tooltip on the virtual CPUs doesn't seem to have the content lined up correctly.

  • Create button looks good.

Create Step

  • In progress looks good.

  • Success looks good.

  • Error looks good (easiest way is to create a VM with the same name as an existing VM).

  • "View the VM" and Close are good and work as expected

sjd78
sjd78 reviewed Jun 3, 2022
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

All of the PF4 changes and the table refactoring look really really nice.

I was worried about loosing any of the disks type handling etc, but I realized that most of that is handled in the sagas and the Thin/Pre dropdown helps drive the underlying disk attributes. All of the wizard code LGTM.

src/components/utils/disks.test.js Outdated Show resolved Hide resolved
src/components/CreateVmWizard/steps/Storage.js Outdated Show resolved Hide resolved
rszwajko added 2 commits June 6, 2022 10:06
@rszwajko
Migrate VM List screen to PF4
9d5f621 
Includes:
1. toolbars
2. tooltips
3. spinner
4. masthead icons

Fixes:
1. use translation independent filters
2. allow filter multiselection
@rszwajko
Include scrollbar width in VM List container padding
d94c13a
@ovirt-infra
Copy link

All tests passed

1 similar comment
@ovirt-infra
Copy link

All tests passed

@rszwajko
Copy link
Member Author

rszwajko commented Jun 6, 2022

@sjd78

Basic Settings Step

I can change the Memory before selection cluster/source/template or iso/OS but I can't change the total CPUs. They should both be read only if other data needs to be selected first (e.g. to get the max counts per cluster level).

This seems not a regression so I would rather fix it under a separate PR.

The Cluster shows as in error if one isn't selected but only if a name is entered. Not sure if "invalid" is the best choice, but it is reasonable. Maybe just start the form with an error on the name and go from there? No need to rehash the "direct the use through the form" debate we had a few years ago.

Done. Validation state was not passed to TextInput. Note that focus styling covers (partially) error styling.

The Cloud-init / sysprep section title seems crowded up against the enable checkbox. Some top and bottom margins like in the PF3 version would look better.

Done. Extra margin was added to sub-heading. Note that this looks best after PF3 styling was removed (until then there is double margin).

Would be nice to had a slightly taller content area for the form to avoid scrolling, but it's manageable.

I agree. However the default Modal used by the Wizard is already ModalVariant.large which seems the largest available. Increasing the size even more requires more research.

As noted in the VM Details Details card, the Operation System dropdown needs to scroll. It is painful to have to scroll the content area instead of the select box. Same thing would probably happen with a lot of clusters or ISOs or Templates. Seems like it would be a generic select box issue.

Not a regression.

Networking Step

Maybe the only thing would be static column widths, as the widths tend to jump around between view, edit, save.

This might be tricky - I'd rather move it to another PR.

Review Step

I miss the "Review and confirm setting" heading.

Done

rszwajko added 4 commits June 6, 2022 18:42
@rszwajko
Re-enable sorting via toolbar icon
dfdd1f1
@rszwajko
Keep the xpath selector used by OST to check VM count
b206d6f 
Integration tests (OST) expect VM count under specific xpath:
'//div[@Class='col-sm-12']/h5'

Preserve this path:
1. keep the class 'col-sm-12' as marker class
2. keep the structure: <div> followed by <h5>
@rszwajko
Migrate Create VM Wizard to PF4
f7d5097 
Key changes:
1. reimplement Network and Storage steps using TableComposable
2. add 'final' step after the Summary for tracking progress
@rszwajko
Add title to SummaryReview step
dea1f5b 
Bring back the title used previously in the PF3 version.

Includes styling changes in BasicSettings step:
1. re-enable validation for VM name
2. add more space above sysprep/cloud init section
@ovirt-infra
Copy link

All tests passed

sjd78
sjd78 approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

LGTM!

@sjd78 sjd78 merged commit 26ac92c into oVirt:master Jun 7, 2022
sjd78 added a commit to sjd78/ovirt-engine-nodejs-modules that referenced this pull request Jun 8, 2022
@sjd78
add preseed for ovirt-web-ui PR 1563
c849025 
Pull request to preseed: oVirt/ovirt-web-ui#1563

Removed stale preseeds for:
  - oVirt/ovirt-web-ui#1549
  - oVirt/ovirt-web-ui#1564
@sjd78 sjd78 mentioned this pull request Jun 8, 2022
Merged
michalskrivanek pushed a commit to oVirt/ovirt-engine-nodejs-modules that referenced this pull request Jun 9, 2022
@sjd78 @mrkev-gh
add preseed for ovirt-web-ui PR 1563
558ab56 
Pull request to preseed: oVirt/ovirt-web-ui#1563

Removed stale preseeds for:
  - oVirt/ovirt-web-ui#1549
  - oVirt/ovirt-web-ui#1564
sjd78 added a commit to sjd78/ovirt-web-ui that referenced this pull request Jun 9, 2022
@sjd78
Expire translations after zanata push
7c3624e 
After merging PRs and pushing their string changes, pull the
translations.  This picks up translation invalidations (English text
has changed), helping to make sure the translations are not wrong.

PRs to consider:
  - oVirt#1533
  - oVirt#1537
  - oVirt#1539
  - oVirt#1540
  - oVirt#1543
  - oVirt#1549
  - oVirt#1564
  - oVirt#1584
  - oVirt#1585 ** pending merge
  - oVirt#1592 ** pending merge
@sjd78 sjd78 mentioned this pull request Jun 9, 2022
Merged
sjd78 added a commit to sjd78/ovirt-web-ui that referenced this pull request Jun 9, 2022
@sjd78
Expire translations after zanata push
af3a4c2 
After merging PRs and pushing their string changes, pull the
translations.  This picks up translation invalidations (English text
has changed), helping to make sure the translations are not wrong.

PRs to consider:
  - oVirt#1533
  - oVirt#1537
  - oVirt#1539
  - oVirt#1540
  - oVirt#1543
  - oVirt#1549
  - oVirt#1564
  - oVirt#1584
  - oVirt#1585 ** pending merge
  - oVirt#1592 ** pending merge
sjd78 added a commit to sjd78/ovirt-web-ui that referenced this pull request Jun 14, 2022
@sjd78
Expire translations after zanata push
eafeb05 
After merging PRs and pushing their string changes, pull the
translations.  This picks up translation invalidations (English text
has changed), helping to make sure the translations are not wrong.

PRs to consider:
  - oVirt#1533
  - oVirt#1537
  - oVirt#1539
  - oVirt#1540
  - oVirt#1543
  - oVirt#1549
  - oVirt#1564
  - oVirt#1584
  - oVirt#1585
  - oVirt#1592 ** pending merge
sjd78 added a commit to sjd78/ovirt-web-ui that referenced this pull request Jun 14, 2022
@sjd78
Expire translations after zanata push
aabcf3f 
After merging PRs and pushing their string changes, pull the
translations.  This picks up translation invalidations (English text
has changed), helping to make sure the translations are not wrong.

PRs to consider:
  - oVirt#1533
  - oVirt#1537
  - oVirt#1539
  - oVirt#1540
  - oVirt#1543
  - oVirt#1549
  - oVirt#1564
  - oVirt#1584
  - oVirt#1585
  - oVirt#1592
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjd78 sjd78 sjd78 approved these changes

@sgratch sgratch Awaiting requested review from sgratch

Assignees
No one assigned
Labels
PF4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rszwajko @ovirt-infra @sjd78