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

Switch to PF4 Modal #1537

Merged
merged 4 commits into from
May 26, 2022
Merged

Switch to PF4 Modal #1537

merged 4 commits into from
May 26, 2022

Conversation

rszwajko
Copy link
Member

@rszwajko rszwajko commented Nov 22, 2021 •
edited
Loading

Depends on PR #1533

  1. NavigationConfirmationModal
    • use titleIconVariant='warning' instead of alert
    • use "lead" class on first paragraph for consistency
  2. About modal
  3. VmConsoleInstructionsModal
  4. VmDetails related modals: Snapshots, Nics, Discs
  5. Replace PF3 MessageDialog: HotPlutConfirmationModal, NextRunChangeConfirmationModal, RestoreConfirmationModal, DeleteConfirmationModal, SessionActivityTracker

@rszwajko rszwajko requested review from sjd78 and sgratch November 22, 2021 11:55
@rszwajko
Copy link
Member Author

Screenshots

Shutdown VM

shutdown_vm_pf4
shutdown_vm_pf3

Remove VM

remove_vm_pf4
remove_vm_pf3

About

PF4:
about_pf4
PF3:
about_pf3

New snapshot

snapshot_pf4
snaphot_pf3

Nics

nics_pf4
nics_pf3

Discs

disk_pf4
disk_pf3

Console instructions

console_instructions_pf4
console_instructions_pf3

Save changes

save_changes_pf4
save_changes_pf3

Navigation confirmation

PF4 (without alert)
nanigation_pf4
PF3 vs P3 modal with PF4 alert:
confirmation_drop_changes

@rszwajko rszwajko changed the title Modal pf4 Switch to PF4 Modal Nov 22, 2021
@rszwajko rszwajko force-pushed the modalPF4 branch 2 times, most recently from a4ae73f to 453b335 Compare November 24, 2021 11:59
@rszwajko
Copy link
Member Author

Modals based on PF3 MessageDialog

Hotplug

hotplug_pf4
hotplug_p3

Nextrun

nextrun_pf4
nextrun_pf3

Delete snapshot

deletesnapshot_pf4
deletesnapshot_pf3

Restore snapshot

restoresnapshot_pf4
restoresnapshot_pf3

Delete Nic/Disk (only PF4 versions)

deletedisk_pf4
deletenic_pf4

Sesssion timeout

Variant with new header:
sessiontimeout_pf4_attention
Attempt to fit existing question into the header (unfortunately it's too long even in English):
sessiontimout_pf4_long

@rszwajko rszwajko force-pushed the modalPF4 branch 2 times, most recently from 830788a to 815b96d Compare December 6, 2021 13:20
@rszwajko rszwajko force-pushed the modalPF4 branch 2 times, most recently from a3f9301 to 8264c06 Compare December 9, 2021 14:16
@rszwajko rszwajko mentioned this pull request Jan 12, 2022
Merged
@ovirt-infra
Copy link

All tests passed

@sjd78 sjd78 added the PF4 label Feb 25, 2022
@ovirt-infra
Copy link

All tests passed

@ovirt-infra
Copy link

All tests passed

1 similar comment
@ovirt-infra
Copy link

All tests passed

@rszwajko
Copy link
Member Author

rszwajko commented May 2, 2022

Added PF3-independent CSS for About dialog. The styling is based on existing PF3 CSS to provide similar user experience. Screenshot of updated design below:

image

sjd78
sjd78 requested changes May 18, 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.

For the editor modals (disk, nic at least), keep this in mind: https://www.patternfly.org/v4/components/modal#with-dropdown. We want the drop downs to be able to be able to extend past the modal container instead of making the modal contents scroll.

The majority of the comments are around rewording the titles and content to better follow the modal design guidelines. More specifically with confirmation dialogs. See: https://www.patternfly.org/v4/components/modal/design-guidelines#types-of-modals

src/components/NavigationConfirmationModal/index.js Outdated Show resolved Hide resolved
src/components/About/About.js Outdated Show resolved Hide resolved
src/components/About/About.js Outdated Show resolved Hide resolved
src/components/SessionActivityTracker.js Outdated Show resolved Hide resolved
src/components/VmConsole/VmConsoleInstructionsModal.js Show resolved Hide resolved
src/components/VmModals/DeleteConfirmationModal.js Outdated Show resolved Hide resolved
src/components/VmDetails/cards/DisksCard/DiskImageEditor.js Show resolved Hide resolved
src/components/VmDetails/cards/NicsCard/NicEditor.js Show resolved Hide resolved
src/components/VmDetails/cards/SnapshotsCard/NewSnapshotModal.js Show resolved Hide resolved
src/components/VmDetails/cards/SnapshotsCard/RestoreConfirmationModal.js Outdated Show resolved Hide resolved
@ovirt-infra
Copy link

All tests passed

@rszwajko
Copy link
Member Author

Changed titles

image

image

image

image

image

image

image

rszwajko added 3 commits May 26, 2022 16:49
@rszwajko
Switch to PF4 Modal
8207806 
1. NavigationConfirmationModal
 - use titleIconVariant='warning' instead of alert
 - use "lead" class on first paragraph for consistency
2. About modal
3. VmConsoleInstructionsModal
4. VmDetails related modals: Snapshots, Nics, Discs
@rszwajko
Replace PF3 MessageDialog
3136ae1
@rszwajko
PF3-independent CSS for About dialog
aed5cd3
@ovirt-infra
Copy link

All tests passed

sjd78
sjd78 requested changes May 26, 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.

One more ? to a title and everything looks good.

src/components/VmDetails/cards/DetailsCard/HotPlugConfirmationModal.js Show resolved Hide resolved
@ovirt-infra
Copy link

All tests passed

@ovirt-infra
Copy link

All tests passed

sjd78
sjd78 approved these changes May 26, 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 1cf74e1 into oVirt:master May 26, 2022
sjd78 added a commit to sjd78/ovirt-web-ui that referenced this pull request Jun 3, 2022
@sjd78
Expire translations after zanata push
ddfda08 
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
@sjd78 sjd78 mentioned this pull request Jun 3, 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
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 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