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

Stats: Move to 3 column layout on period pages. #11124

Merged
merged 3 commits into from Feb 28, 2017
Merged

Conversation

timmyc
Copy link
Contributor

@timmyc timmyc commented Feb 2, 2017

This branch closes #11094

This branch adds logic to support 3 columns on stats period pages ( Day, Week, Month, Year )

stats_ _fly_fishers_place_ _wordpress_com

I explored a few different pure flexbox solutions, but due to the varying heights of the stats modules ( because of data / lack of data ) this proved to be a difficult path to take. I do welcome any others who are more flex-box gifted than I to experiment further though!

The site.jsx diff looks confusing, but essentially I have moved the modules into 3 different .stats__module-columns. I wanted to make sure the Countries and Post & Page modules were still shown at the top in both 2 and 3 column views.

I also experimented with bumping the <Main /> max-width to 1280px and the 3 column layout looked great at this resolution too. Insights page though really needs to be updated before we bump all of stats up to 1280.

The other minor change was I added the hover state to all rows in stat modules. Rows without a primary action ( like in Countries ) did not have a hover state.

/cc @folletto

I have tested this only in Chrome and the chrome device emulator - I need to spend some time with this branch on some devices and other browsers too.

@matticbot
Copy link
Contributor

matticbot commented Feb 2, 2017

@timmyc timmyc added Stats Everything related to our analytics product at /stats/ [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 2, 2017
@timmyc
Copy link
Contributor Author

timmyc commented Feb 2, 2017

And /cc @m if you want to take the branch for a spin.

@folletto
Copy link
Contributor

folletto commented Feb 2, 2017

Checked on Chrome with the calypso.live link above.
Design wise is good. Three columns work well on wide screens. 👍

Echoing your analysis, I'm wondering too if it makes more sense to bump this to the next breakpoint from >1040px to >1280px. It would still allow the three columns as .is-wide-layout is 1040px which means that it triggers at a window width of 1390px, which means that everyone between 1040px and 1390px width would get 3 columns, and below the two columns seem fitting without getting too cramped.


For reference, this is the smallest size with breakpoint at >1040px:

And this is the smallest size with breakpoint at >1280px:

It's not too bad either way, I think the bottom one is more effective.

@youknowriad
Copy link
Contributor

youknowriad commented Feb 2, 2017

This looks good 👍 but for the two columns layout, the second column seems too small comparing to the first one.

Maybe we could consider using a library here https://github.com/callmecavs/bricks.js I also found this after a quick search https://tsuyoshiwada.github.io/react-stack-grid/#/

@timmyc
Copy link
Contributor Author

timmyc commented Feb 2, 2017

Maybe we could consider using a library here

I'd be up for exploring those in another branch. Looking at the demos, I fear that stat modules might be randomly placed on the page depending on the amount of data. So for one period, Clicks might show up in one location, but on another with more traffic it would change. But these are just hunches based upon a quick glance at the code and the demos - so if we have time it would be neat to look into them both.

the second column seems too small comparing to the first one.

Indeed - we need to consider this a bit more with this "static" 3 column layout. Ultimately, if we could give back the ability to set a custom sort order I think that would be the best solution.

@timmyc
Copy link
Contributor Author

timmyc commented Feb 2, 2017

Applied updates per feedback in 97da65c - also moved around some modules to try to make a good mix in height between 2 and 3 columns, and most "used" modules. Again - if we could provide the ability to customize order of modules, that would be the best solution there.

Other minor change is reducing the padding between modules to better match the new gutter sizes:

3 columns
stats_ trout_bummin _wordpress_com

2 Columns
stats_ trout_bummin _wordpress_com

The map really does make the 2nd column more vertically heavy.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Comparing to what we currently have, this is a good addition for users with large screens. For the two columns layout, it's not that bad.

Let's ship this 👍 and if we have some time before our meetup, we could try using one of the masonry layout libs.

Also, there's another option is the usage of something like https://github.com/AlecAivazis/redux-responsive and switch the layout programmatically depending on the window width.

Copy link
Contributor

@folletto folletto left a comment

Choose a reason for hiding this comment

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

Tested on Chrome and Firefox, design wise 👍 too!

@matticbot
Copy link
Contributor

@timmyc This PR needs a rebase

@matticbot matticbot added the [Size] L Large sized issue label Feb 27, 2017
@matticbot matticbot added the [Size] L Large sized issue label Feb 28, 2017
@timmyc timmyc merged commit 4a055ec into master Feb 28, 2017
@matticbot matticbot removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 28, 2017
@alisterscott alisterscott deleted the try/stats/masonry-layout branch October 16, 2017 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Size] L Large sized issue Stats Everything related to our analytics product at /stats/ [Status] Needs Rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stats: Add Breakpoint on Period Pages to flow to 3 Columns
4 participants