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

Modernize pagination markup to HTML5 and switches deprecated listingB… #15

Merged
merged 4 commits into from
Jan 21, 2016

Conversation

davilima6
Copy link
Member

…ar CSS class reference to pagination

@thet
Copy link
Member

thet commented Jan 20, 2016

@davilima6 reviewed, tested and fixed!
please review my changes.
let's wait for the jenkins job to finish.

@thet thet force-pushed the pagination_markup branch 2 times, most recently from b80b46b to 046562a Compare January 20, 2016 11:15
@thet
Copy link
Member

thet commented Jan 20, 2016

The brackets broke the visual design - the spans btw li elements are designed to look like the anchor elements, but the brackets were outside the spans... Of course, that could be solved, but I think, they are not needed. I'd add them with CSS :before and :after selectors, I'd want them.

@thet thet force-pushed the pagination_markup branch from 046562a to a86ec08 Compare January 20, 2016 19:13
@thet
Copy link
Member

thet commented Jan 20, 2016

@hvelarde - as someone concerned about migration issues - do you have an opinion about this?
if ok, plz merge.
it's definitely a step forward, alone that the listing bar is finally styled in plone 5! (wasn't before...)

agitator added a commit that referenced this pull request Jan 21, 2016
Modernize pagination markup to HTML5 and switches deprecated listingB…
@agitator agitator merged commit 1af82c2 into master Jan 21, 2016
@agitator agitator deleted the pagination_markup branch January 21, 2016 09:35
thet added a commit to plone/buildout.coredev that referenced this pull request Jan 21, 2016
@hvelarde
Copy link
Member

:-) just give me some time; I think we have a requirement related with navigation from one of our customers... I'll be back with some feedback.

@hvelarde
Copy link
Member

@thet seems that I'm already late but... there is a request for one of our customers of something that seems related with this: usage of <link rel="prev" href="..." /> and <link rel="next" href="..." /> on listings and the consequent removal of <link rel="canonical" href="..." />; do we open a new issue?

@hvelarde
Copy link
Member

@agitator please ask always for a squash, when needed, before merging

@thet
Copy link
Member

thet commented Jan 21, 2016

I'm not always pro squashing - one, I'd have removed @davilima6 from commit history and second some commits should stand for itself to show the reason of a particular change (but not really in this case).

your suggestion about <link rel> - that's a different issue / pull request. regarding to https://googlewebmastercentral.blogspot.co.at/2011/09/pagination-with-relnext-and-relprev.html the link elements should be injected into the html/head section. why doesn't your customer want rel="cannonical"? sounds like a good fit.
all in all, i'm not sure, if that's still best practice or is this something google tried to establish couldn't enforce? but this should be discussed in a different issue.

@hvelarde
Copy link
Member

@thet that's why I said "when needed".

on the other side, you're right about the <link rel="canonical" href="..." />, there's no need for removal; I'll create a feature request.

@thet
Copy link
Member

thet commented Jan 21, 2016

@hvelarde it's a different issue, but regarding your link next/previous thing: this can be made optional, like the ISocialMediaSchema controlpanel options. It's even a bit related to that, because it changes visibility to other services (google).

@hvelarde
Copy link
Member

@thet I don't think it has to be optional; it makes no harm and is common among search engines; I opened a new issue in plone/Products.CMFPlone#1325

@mauritsvanrees
Copy link
Member

I am making releases. Currently Plone 4 is using the master branch of plone.batching. Since the version number has been bumped to 1.1 and there is an upgrade warning at the top of the changelog file, it seems better to create a maintenance branch 1.0.x for Plone 4, without these changes.

@davilima6 Do you agree?

@thet
Copy link
Member

thet commented Feb 12, 2016

@mauritsvanrees I agree. In Plone 4.x only plone.batching < 1.1 should be in. If someone wants - like me - then it's always possbile to use 1.1 in Plone 4.x by explicitly depending on it.

@davilima6
Copy link
Member Author

@mauritsvanrees: +1

@mauritsvanrees
Copy link
Member

Good. I have created branch 1.0.x.
And the changes above are in fresh release 1.1.0.
Thanks for your work on this!

@davilima6
Copy link
Member Author

Thank you, @mauritsvanrees. Could you please release 1.0.8 today as well or should we better wait Plone 4.3.8 to get closer?

@mauritsvanrees
Copy link
Member

There are no changes on the 1.0.x branch, except one minor edit in setup.py. So no release is needed.

@davilima6
Copy link
Member Author

@mauritsvanrees Actually I believe this commit b0a5673 should have been the tip for 1.0.x branch, as explained in #14 (comment). Should I just cherry-pick it there or would you recommend anything else?

@thet
Copy link
Member

thet commented Feb 13, 2016

@davilima6 right. It's mentioned in the 1.0.8 section of the CHANGES.rst file in the master branch: https://github.com/plone/plone.batching/blob/master/CHANGES.rst
Your cherry-pick should fix it.

@mauritsvanrees
Copy link
Member

Ah, I wasn't aware of that separate pull request. I will cherry-pick the commit and release 1.0.8 for Plone 4.3.

@mauritsvanrees
Copy link
Member

1.0.8 released.

@davilima6
Copy link
Member Author

Thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants