-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Stats: Update module headers to SectionHeader #2863
Conversation
Good job updating these! I have a couple comments:
Other than those comments, it's looking good. The modules look much cleaner! |
"Posting activity" on background instead of white created a nice variation in style, but it's just a personal preference. :) Apart from that, I like the change. :) |
Another minor thing I noticed are the extra empty squares on the Posting Activity. |
31c61b4
to
700568b
Compare
5cf0ba4
to
f310415
Compare
@dmsnell can i get a review on this? |
75e98d7
to
0f294ba
Compare
<SectionHeader label={ this.translate( 'All-time posts, views, and visitors' ) }></SectionHeader> | ||
<Card className={ classNames( 'stats-module', 'stats-all-time', classes ) }> | ||
<div className="module-content"> | ||
<div className="module-content-text module-content-text-info"> |
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.
If the info toggles are no more, we can rip out these div's. Sounds like we should do a follow up PR and remove all info / toggle logic once this PR lands.
A bit more sweeping done on this branch. After this is merged we can do away with the 4909647 - Removed info boxes / toggle logic from various components From our discussion in slack, we are going to punt for now on removing the (i) icons from the post summary pages. @alternatekev and @jancavan if you could both give this branch another test drive after these updates that would be excellent. |
Also want to summarize some of the big changes in the branch for anyone wanting to check it out:
Note: Only gridicon now links to summary pages ( when available ) |
Thanks for wrangling this @timmyc and @alternatekev. The section header looks nice and clean. I think everything else looks good, but I don't think we should be removing "View all". It's not obvious that clicking on the graph icon takes you to the entire list and it's also really not the right symbol for it - a graph that takes you to a list item. The module heading should at least be clickable if we're removing "View all", but my vote is to add it back. |
@alternatekev shall I add the footer/view all back in? |
I’m walking through this with Sheri today in order to get a better feel for where she thinks users will get tripped up. I’d almost like to investigate using a clickable compact Card instead of SectionHeader here since that uses the full bar to click and has a more appropriate default icon (the right-facing chevron). |
Updating the status until we have some direction on that. |
sorry @alternatekev I don't think I'll be able to right now |
@alternatekev & @jancavan "View All" links added back in 4032165 |
@alternatekev if you don't mind doing one last test pass then we can get this merged in ( finally ). |
modified stats-module to use sectionheader css fixes to module sectionheaders module text padding updated insights to use section-header css fixes to module sectionheaders module text padding fixed issues with the scrolly part of the post trends visualization updated insights to use section-header modified stats-module to use sectionheader css fixes to module sectionheaders module text padding updated insights to use section-header css fixes to module sectionheaders module text padding fixed issues with the scrolly part of the post trends visualization updated insights to use section-header css fixes to module sectionheaders module text padding updated insights to use section-header css fixes to module sectionheaders module text padding fixed issues with the scrolly part of the post trends visualization updated insights to use section-header modified stats-module to use sectionheader css fixes to module sectionheaders module text padding updated insights to use section-header css fixes to module sectionheaders module text padding fixed issues with the scrolly part of the post trends visualization fixes to sectionheaders & view all buttons/icons es6 upgrades and formatting cleanup es6 upgrades and formatting cleanup removed extraneous left/right arrows from post-trends added use of containerClass var fixed scroll bug on post-trends added borderless prop to stats-tabs to remove shadow effect from insights page removed extra icon removed extra site icon Removing unused toggle/info logic. Fix up lint errors. Use props.period to handle instances where modules do not have summary pages. Clean up header on followers page. Add View All links back in. changed chart icon to chevron-right
# The first commit's message is: updated sectionheaders to use href prop instead of action buttons # This is the 2nd commit message: removed extraneous vars
3f4981f
to
a6623fe
Compare
Tested Video walkthrough: 7m51s Nitpicks/Observations
Most of this list is out of scope for this PR, so I can raise issues separately for any of them that are worth moving forward. Moving the email followers download button to top right is probably the only suggestion I'd make for including now if it can be updated in this round of changes. Even though users may miss the "View All" links because they are used to it, I think we could still try removing it. A temporary explanation popover could help die-hard stats lovers transition to the more stream-lined design without needing the "View All" link—and then we could test our assumptions with a few user tests to see if any new users cannot figure out how to get to summary pages. |
I think this would be totally worth trying too, perhaps in a follow-up PR though. There are some big changes afoot here already - namely removing of the collapse toggle for modules ( which were persisted in localStorage ), and to some extent the (i) buttons - though those probably aren't used by power users of stats. My biggest concern with making too many changes here is the fact that there is not a team dedicated to working on stats right now. I think that even this PR will potentially generate some feedback that might require follow-up PRs. All of your ideas/issues outlined above are great feedback and really should be worked on, but until we can dedicate some dev/design resources around this we need to be careful to not change too much. |
@timmyc, would you be willing to remove "View All" links now and just revert sooner than later if users complain too much? (I can help track user feedback for this.) |
Sounds good to me, but we also need to give ti some time to get a good amount of feedback. Also need to give a heads up to all HE's as I think there will be questions around the change. |
@timmyc I've moved the Email Followers download button into the SectionHeader. |
LGTM. I think we should get this out the door and follow up with the view-all links. |
Thanks for the thorough review @designsimply. I'm still leaning towards keep "View all", but I think the new change to the chevron icon is much more intuitive than the previous version.
This has always been like this (I think) even before it was ported over and we didn't get around to make this better and yes, it's confusing. |
A user noted of a bug with "View all", maybe it's related to some changes here? See #4507. |
New issue raised for this: #5162 |
This PR takes the current Stats modules and standardizes on the SectionHeader components:
cc @jancavan