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

Replace PF3 Alerts #1533

Merged
merged 2 commits into from
May 18, 2022
Merged

Replace PF3 Alerts #1533

merged 2 commits into from
May 18, 2022

Conversation

rszwajko
Copy link
Member

@rszwajko rszwajko commented Nov 9, 2021 •
edited
Loading

This PR depends on #1525

Replace existing PF3 Alerts with newer PF4 widgets.
Key points:

  1. PF4 requires a title (which makes sens from usability point of view). However some our alerts have no title which makes them look bad after migration. Where possible a generic title was added i.e. "Messages" or "Details"
  2. PF4 styling favors displaying alerts above the text (i.e. bold line at the top of the alert box). In one case (error on VM Details screen) we display alert at the bottom.
  3. PF4 introduces self-dismissing alerts which makes our CounterAlert obsolete. However for now the original design was used.
  4. in PF4 ToastNotifications are also plain Alerts - this will be handled in separate PR.

@rszwajko
Copy link
Member Author

rszwajko commented Nov 9, 2021

Screenshots (PF4 above, PF3 below):

settings_success
settings_failure
confirmation_drop_changes
summary_review_success
create_snapshot_info
vm_details_error
vm_details_pending_changes

@rszwajko rszwajko requested review from sjd78 and sgratch November 9, 2021 13:48
@rszwajko
Copy link
Member Author

Screenshots for toast notifications

existing PF3

toast_notifications_PF3

new PF4 notification (with title)

toast_notifications_PF4

new PF4 notification with generic title.

The generic title is just a fallback for messages that provide no title. Missing title can be added later on in a follow up PR (in some cases this requires splitting the message into 2 parts which will invalidate translations).
toast_notificattions_PF4_with_generic_title

@rszwajko rszwajko mentioned this pull request Nov 22, 2021
Merged
@rszwajko
Copy link
Member Author

Note that alerts in "Create snapshot" modal and "VM has pending configuration..." can be replaced with Hint component.
See PR #1540 for new screenshots.

@rszwajko rszwajko force-pushed the alertPF4 branch 2 times, most recently from 245621b to 4e19d2a Compare December 6, 2021 13:19
@rszwajko rszwajko force-pushed the alertPF4 branch 2 times, most recently from 035b466 to be3b751 Compare December 9, 2021 14:15
@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

sjd78
sjd78 reviewed May 11, 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.

Only a few comments, but looks pretty good.

src/components/CounterAlert.js Outdated Show resolved Hide resolved
src/components/CreateVmWizard/steps/SummaryReview.js Show resolved Hide resolved
src/components/ToastNotifications.js Show resolved Hide resolved
@ovirt-infra
Copy link

All tests passed

sjd78
sjd78 approved these 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.

LGTM

@sjd78 sjd78 merged commit ef4bf64 into oVirt:master May 18, 2022
@sjd78 sjd78 mentioned this pull request May 18, 2022
Merged
@sgratch
Copy link
Member

sgratch commented May 18, 2022

@sjd78 @rszwajko need to open another dup PR for that since we do want to merge it later....

@sjd78 sjd78 mentioned this pull request May 26, 2022
Merged
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.
4 participants
@rszwajko @ovirt-infra @sgratch @sjd78