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.

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

Display help icons for categories/subcategories #17062

Merged
merged 19 commits into from Feb 10, 2021
Merged

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Jan 8, 2021

Description:

Adds a new notification type (not made public or advertised) for category/subcategory documentation. Also changes the styling for the in-report help icon, but doesn't make it permanently visible. Might be a good idea to do that I guess, but I wasn't sure.

Fixes #13716

image

Review

  • Functional review done
  • 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

@diosmosis diosmosis added this to the 4.2.0 milestone Jan 8, 2021
@diosmosis diosmosis added the Needs Review PRs that need a code review label Jan 8, 2021
@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Jan 8, 2021
@diosmosis diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jan 12, 2021
@diosmosis
Copy link
Member Author

@mattab @tsteur @sgiehl this is ready for review (code + UI)

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.

Left a minor comment. Everything else seems to work as expected. But might be good if @mattab or @tsteur would also have a look at the colors and the new help texts to ensure they fit

plugins/CoreHome/Categories/VisitorsCategory.php Outdated Show resolved Hide resolved
@diosmosis
Copy link
Member Author

@mattab / @tsteur can you provide a UI review?

@tsteur
Copy link
Member

tsteur commented Jan 18, 2021

@diosmosis currently, it shows the help icons for the currently selected ones
image

The help icon on the main category talks about the reporting page but probably needs to talk about the "visitors report pages"
image

I don't know if it's easily possible but be great to adjust the titles like below

  • How does the "Visitors" reporting section help me? // replace Visitors and visits log with the category/page name..
  • How does the "Visits Log" reporting page help me?

If it's too complicated can also not do it.

And generally I wonder if it was better to only show the help icons for the currently hovered reporting category/page. Because we then adjusted the text to not refer to the current reporting page this would be possible. Of course it might be confusing to see a report help for one page in the context of a different page so we could also additionally switch the page and show the help text at once. That's just a thought though because I found it visually bit cluttered when hovering another menu item and the other two question marks are still shown. Eg here I hover "real time map" and I wonder if the other two question marks should disappear and it should be instead only be shown for the real time map.
image

A problem be though that when clicking on the reporting category there is no page to show so maybe it doesn't work. For sections we could otherwise just show it as part of the current page though maybe. Personally, I would maybe show the help icon only for reporting pages.

Waiting for the thoughts from @mattab

@diosmosis
Copy link
Member Author

If it's too complicated can also not do it.

It's do-able.

Of course it might be confusing to see a report help for one page in the context of a different page so we could also additionally switch the page and show the help text at once.

Switching the page is a possibility. I can try this and we can revert if it's weird. Though actually, seeing a help icon on hover for every category on the left might be weird too... I guess we can do it on hover of a single menu item. I'll try this.

Personally, I would maybe show the help icon only for reporting pages.

Makes sense for a first version.

@mattab
Copy link
Member

mattab commented Jan 20, 2021

The UI looks good overall 👍 (feedback below)

Personally, I would maybe show the help icon only for reporting pages.

Agreed 👍 the help icon can be removed from the top menus and only show for the current reporting page (on hover).

Also a small note but in terms of UX, because the icon is tiny (good for visual effect), it's (very) hard to click on it.
Could we make the clickable area bigger around icon, so it's easier to hover on it and click it? (if i had to guess, making clickable area 2x wider and 2x higher could work)

Also on hover on the icon, could we maybe show the colored icon? (to show some feedback and hint that clicking might do something). (similar to how it shows blue when hovering the icon next to a report's title)

@diosmosis
Copy link
Member Author

@mattab / @tsteur applied all feedback

@sgiehl
Copy link
Member

sgiehl commented Jan 20, 2021

btw. should we at some point mention those category classes on https://developer.matomo.org/guides/menus ?

@tsteur
Copy link
Member

tsteur commented Jan 20, 2021

👍 now only need to wait for the help texts. We could already merge though and them later to avoid more merge conflicts later

@sgiehl
Copy link
Member

sgiehl commented Jan 26, 2021

Some tests are failing due to the changes. @diosmosis could you update them so we can get that merged?

@diosmosis
Copy link
Member Author

The help texts are almost done I think. We can wait until then and I'll set them in this PR so I don't have to update expected test files twice.

@diosmosis diosmosis merged commit bc1d57f into 4.x-dev Feb 10, 2021
@diosmosis diosmosis deleted the 13716-page-help-2 branch February 10, 2021 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show a message in each most valuable pages in Matomo to explain why it's important
4 participants