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

icon for informational system check #17389

Merged
merged 3 commits into from Mar 25, 2021
Merged

icon for informational system check #17389

merged 3 commits into from Mar 25, 2021

Conversation

Findus23
Copy link
Member

@Findus23 Findus23 commented Mar 24, 2021 •
edited by sgiehl

This is quite subjective, but I think it looks nicer if there is also an icon for informational entries

Before:
grafik

Now:
grafik

One could use icon-info instead as it looks a bit nicer, but it doesn't fit the style of the other icons.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@Findus23 Findus23 added c: Design / UI For issues that impact Matomo's user interface or the design overall. Needs Review PRs that need a code review labels Mar 24, 2021
@Findus23
Copy link
Member Author

Also there don't seem to be a LESS variable for the error/warning/info/sucess colors, so I copied the ones from notifications over.

If you are interested I could create a PR unifying them over all of Matomo.

@flamisz
Copy link
Contributor

flamisz commented Mar 24, 2021

These are the before-after screenshots, to make it easier to decide which one is preferred:

Screen Shot 2021-03-25 at 8 43 41 AM

Screen Shot 2021-03-25 at 8 44 20 AM

@Findus23
Copy link
Member Author

If someone doesn't like the icons in the informational results we could maybe remove them just there

sgiehl
sgiehl approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

code looks fine

@sgiehl
Copy link
Member

sgiehl commented Mar 25, 2021

Personally I like the change. Maybe @tsteur or @mattab could state their opinion as well.
@Findus23 Guess the expected UI files would need an update before we could merge it...

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 25, 2021
@tsteur
Copy link
Member

tsteur commented Mar 25, 2021

All good 👍

@Findus23
Copy link
Member Author

Just noticed while updating tests that there is a separate CSS file for the installer.

@diosmosis diosmosis added this to the 4.3.0 milestone Mar 25, 2021
@diosmosis diosmosis merged commit 9e265af into 4.x-dev Mar 25, 2021
@diosmosis diosmosis deleted the info-systemcheck branch March 25, 2021 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
c: Design / UI For issues that impact Matomo's user interface or the design overall.
Projects
None yet
Milestone
4.3.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Findus23 @flamisz @sgiehl @tsteur @diosmosis