Skip to content
This repository has been archived by the owner on Jun 16, 2020. It is now read-only.

Pagination margin #807

Closed
wants to merge 4 commits into from
Closed

Pagination margin #807

wants to merge 4 commits into from

Conversation

sboerrigter
Copy link
Member

Fixes #777

Copy link
Member

@haroldangenent haroldangenent left a comment

Choose a reason for hiding this comment

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

Maybe you should use @luukdv's suggestion from #777? See bug with this approach below.

schermafbeelding 2017-09-07 om 09 52 36

@sboerrigter
Copy link
Member Author

sboerrigter commented Sep 7, 2017

Sorry, I overlooked @luukdv about the negative margin on the wrapper. Unfortunately this solution won't work because the margin-bottom property gets overwritten by:

main .wrapper>:last-child {
    margin-bottom: 0;
}

I solved it by changing the pagination breakpoint and showing less pages in the pagination by default.

@luukdv luukdv removed their request for review September 7, 2017 14:19
@@ -31,7 +31,7 @@
.prev {
float: left;

@include respond-to('medium') {
@include respond-to('large') {
Copy link
Member

Choose a reason for hiding this comment

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

Generally speaking, I don't really like this solution. Just because @luukdv's theoretical proposal causes a practical issue (specifity issue in this case), I don't think this should be solved this way. I'd recommend dealing with the specifity issue.

In this case, showing pagination on screens smaller than 981px could come in handy for users.

$context['pagination'] = Timber::get_pagination([
'mid_size' => 2,
'mid_size' => 1,
Copy link
Member

Choose a reason for hiding this comment

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

I personally also preferred a mid_size of 2 (in most cases this looks and works better, because there are often 6-7 pages that don't need to be split up necessarily), but that's just my opinion.

@sboerrigter
Copy link
Member Author

I will close this PR since it is not a good solution for the issue. I have no better solution right now, so I will remove the issue from the milestone for this release as well.

@sboerrigter sboerrigter closed this Oct 6, 2017
@sboerrigter sboerrigter deleted the pagination branch October 6, 2017 08:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants