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

fix: consistent pagination layout #3457

Merged
merged 1 commit into from Oct 16, 2017

Conversation

Projects
None yet
5 participants
@tsl143
Copy link
Contributor

commented Oct 12, 2017

Fixes #3436

Before:
screen shot 2017-10-12 at 5 51 37 pm

After:
screen shot 2017-10-12 at 5 59 15 pm

@willdurand

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

Saw your comment in the issue, I personally find this is too big on desktops, and I'd go for the other way around actually. It makes sense to have pagination below the list only in my opinion, and it does not affect the mobile view anyway.

@codecov-io

This comment has been minimized.

Copy link

commented Oct 12, 2017

Codecov Report

Merging #3457 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3457   +/-   ##
=======================================
  Coverage   95.84%   95.84%           
=======================================
  Files         178      178           
  Lines        3464     3464           
  Branches      697      697           
=======================================
  Hits         3320     3320           
  Misses        124      124           
  Partials       20       20

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c26a53a...3eed17e. Read the comment docs.

@tsl143

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

Saw your comment in the issue, I personally find this is too big on desktops, and I'd go for the other way around actually. It makes sense to have pagination below the list only in my opinion, and it does not affect the mobile view anyway.

@willdurand, yes that's the way it should be, shall we wait for @pwalm, or already implement this?

@willdurand

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

This is going to be updated post-release anyway (#3100), so I'd just go ahead but maybe @tofumatt has some more thoughts on it before you change anything again :)

@muffinresearch

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

@tsl143 I agree with Will - If it's not too much trouble to do the opposite then having the pagination under the content (like the first screenshot) on balance looks better.

@willdurand willdurand force-pushed the tsl143:I3436 branch from 3c4b8e1 to e4dfad6 Oct 16, 2017

@willdurand willdurand force-pushed the tsl143:I3436 branch from e4dfad6 to 3eed17e Oct 16, 2017

@willdurand

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

Hi @tsl143, I went ahead and adjusted your PR 😉

@willdurand

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

Updated screenshot for the search results:

screen shot 2017-10-16 at 19 07 42

@willdurand willdurand requested a review from tofumatt Oct 16, 2017

@willdurand

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

@tofumatt since I modified @tsl143's PR, I'd like a quick r? Thanks!

@tofumatt
Copy link
Member

left a comment

wfm!

@willdurand willdurand merged commit 5c9a1ef into mozilla:master Oct 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.