Make ellipses stylable and provide more CSS hooks to pagination markup

[davilima6]

No description provided.

[thet]

Great! Only concern: the removed class listingBar will cause design breaks in deployments. But I agree, pagination is a much better wording for this.

When doing changes here, what about changing the structure for a better semantic meaning? Probably a <nav> tag with <ul>/<li> listing would be a better fit, nowadays. But cause even more damage to existent deployments... :/

[davilima6]

You're right, listingBar should be kept in this template or else we should change all occurrences in JS and CSS, which are:

CSS/JS references to .listingBar: 6

• archetypes/referencebrowserwidget/skins/referencebrowser/referencebrowser.js
• plone/app/search/search.js
• plonetheme/classic/skins/classic_styles/print.css.dtml
• plonetheme/classic/skins/classic_styles/public.css.dtml
• plonetheme/classic/skins/classic_styles/RTL.css.dtml
• plonetheme/sunburst/skins/sunburst_styles/print.css

CSS/JS references to div.listingBar: 3

• plone/app/content/browser/folder_contents.js
• plone/app/registry/browser/records.pt
• plonetheme/sunburst/skins/sunburst_styles/base.css

Templates using div class="listingBar": 2

• plone/batching/batchnavigation.pt
• plone/app/layout/nextprevious/nextprevious.pt

So I see two paths: either we keep div.listingBar as is or we update it to nav.pagination + ul li in the files above (more risky because of multiple PRs but shouldn't be hard). Opinions?

Re: existing deployments, not completely sure about this case but I think Plone 5 should be the major version where we fix all these old markup issues, shouldn't it? So integrators should already expect a bit more work, alongside Barceloneta changes.

[thet]

Did you search in a Plone 4.3 installation?
In Plone 5, I cannot find any of the referenced files but:

• plone/app/registry/browser/records.pt without the reference to div.listingBar

and

• plone/batching/batchnavigation.pt
• plone/app/layout/nextprevious/nextprevious.pt

I wonder, if nextprevious functionality couldn't be changed to use plone.batching's navigation directly?

With only these few references, it's easier to change it to nav.pagination + ul li.

Existing deployments/projects will still break, if they customize the batch navigation styles and use listingBar. I did such customizations myself in a lot of (Plone 4.3 based) projects.

But if this change is good documented (raise at least minor version, document in CHANGES.rst and the upgrade guide) I'm all for modernizing our HTML layout, which is out of date on a number of places.

[davilima6]

Yep, that was the count for Plone 4.3.7. For P5 coredev it is much smaller:

CSS/JS references to .listingBar: 1

• plonetheme/barceloneta/theme/less/pagination.plone.less

CSS/JS references to div.listingBar: 2

• plone/app/registry/browser/resources/registry.js
• zope/app/basicskin/zopetopwidgets.css

Templates using div class="listingBar": 2

• plone/batching/batchnavigation.pt
• plone/app/layout/nextprevious/nextprevious.pt

So my doubt now is: should I propose PRs to the P5 packages only or to all listed in my previous comment, thus providing the improvements to P4 too? IMO at least the ellipsis separation in its own DOM element is an addition that will be appreciated for the P4 series (many projects still begin with it).

Re: nextprevious, I agree but maybe that change should pertain to another PR?

[thet]

I would add the pagination class additionally to listingBar, merge your whole pull request and add a changelog entry, which states that listingBar is deprecated. This would make a 1.0.8 release, which ships with Plone 4.3. This way, all people would benefit from your changes in this pull request while keeping backwards compatibility.

Next pull request would be to remove listingBar, add the nav/ul/li HTML structure, change references to listingBar and div.listingBar in all Plone 5 core packages and document the changes properly. This would make a plone.batching 1.1 release.

[davilima6]

Thanks for the suggestions! I updated the PR and successfully ran tests locally for plone.batching and Products.CMFPlone. Former's tests don't care for markup, not sure if any Robot ones do. Should I spawn a Jenkins job as well?

[thet]

I've added a Jenkins Job for this pull request. You can see it running below ("Some checks haven’t completed yet"). Should go green if all is OK. This job runs the whole test suite, including Robot tests.

You can add such Pull-Request jobs here: http://jenkins.plone.org/job/pull-request-5.0/build

[davilima6]

Thanks, @thet. It seems we have a green light :)

[thet]

Done! Tnx.