-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Update notifications and info boxes styles for accessibility #21077
Conversation
Created new ticket to look into the consolidation of the components — #21079. |
031b5bd
to
12f5b45
Compare
@bx80 for such intense changes it's good to temporarily update the submodules to the correlating branches in the PR. That way you can directly see if all tests of the submodule plugins are passing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a look through all the changed screenshots. Left a couple of comments, where additional changes might be required.
Some of them might be unrelated to the purpose of this PR and could also be changed independently. In some cases the resulting screenshots aren't correct anymore and need further updates.
In general we should maybe discuss if we want to deprecate either the Notification or the Alert component. It seems they already look the same, so their is no benefit in having both.
plugins/Login/tests/UI/expected-screenshots/Login_bruteforcelog_noentries.png
Outdated
Show resolved
Hide resolved
...place/tests/UI/expected-screenshots/Marketplace_notification_plugincheck_exceededLicense.png
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess we missed that when changing the links, but guess those for authors and website should be blue, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the marketplace .metadata CSS class to make them blue.
...expected-screenshots/Marketplace_superuser_enable_plugins_admin_with_multiserver_enabled.png
Outdated
Show resolved
Hide resolved
plugins/UserCountryMap/tests/UI/expected-screenshots/VisitorMap_bounce_rate.png
Outdated
Show resolved
Hide resolved
plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_permissions_bulk_access_set.png
Outdated
Show resolved
Hide resolved
plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_superuser_tab.png
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we maybe change this help box to look the same as an info notification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bottom right italic text might not work well in an info notification. Maybe there are other improvements that could be done to the report info help box at the same time so it might be worth getting some UX input on another ticket.
tests/UI/expected-screenshots/UIIntegrationTest_category_help.png
Outdated
Show resolved
Hide resolved
… usage to icon-info, added source SVG files for new icons
…eporting actions icon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had another look through the screenshots. Added a couple of comments, where screenshot changed the content unexpected, or some stuff wasn't correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placement of the dropdown icons when displayed right looks not nice. See the dashboard selector in main menu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugins/Live/tests/UI/expected-screenshots/Live_visitor_profile_action_tooltip.png
Outdated
Show resolved
Hide resolved
plugins/Login/tests/UI/expected-screenshots/Login_password_reset_complete.png
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also incorrectly updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
plugins/UserCountryMap/tests/UI/expected-screenshots/VisitorMap_avg_time_on_site.png
Outdated
Show resolved
Hide resolved
plugins/UserCountryMap/tests/UI/expected-screenshots/VisitorMap_bounce_rate.png
Outdated
Show resolved
Hide resolved
Co-authored-by: Stefan Giehl <[email protected]>
Co-authored-by: Stefan Giehl <[email protected]>
a520140
to
3b1874a
Compare
505a253
to
a6b10b8
Compare
a6b10b8
to
a303e9e
Compare
Description:
Fixes #20691
The PR updates the styling of notifications and info boxes throughout the application to improve accessibility.
For some reason we have components for both notifications and alerts, each of which can be of the types: success, info, warning or error. The only real difference I can see is that notifications can optionally have a close button. There seems to be roughly equal usage of both components throughout the application. If there is no other difference then we might want consider deprecating alerts at some point in favor of notifications.
I’ve applied the new styles to both notifications and alerts and also added some notification examples to the UI demo screen so that they can be covered by UI tests.
Review