-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Centralize pagination/canonical meta URL generation in Document #3077
Conversation
Before merging, we'll also want to append ": Page X" on non-first-pages to the page title. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving pending the title change mentioned in the comment above.
68859c2
to
bf84257
Compare
[ci skip] [skip ci]
@tankerkiller125 @SychO9 re-requesting review, as I've refactored the basic title driver to use translations for layout. Specific issues:
|
It's an implementation detail, and can be made available on specific implementations as needed.
Currently, our Document class does not support generating meta prev/next URLs. With this PR, content can set
page
andhasNextPage
on theDocument
object. This, in turn, will appropriately generate next/prev links, and configure the canonical URL properly.Questions for Reviewers:
As part of this, we introduce a helper function to add/override/remove params from URLs. Is this the best place for it?
As an alternative approach, we could create a generic
AbstractPaginatedContent
class with methods, and move logic into there. Then, Document would just have public fields fornextPageUrl
andprevPageUrl
. This would let Document focus on arrangement/presentation, and avoid hardcoding logic for a pagination scheme. On the other hand, if we have centralized, defined logic for a pagination scheme, we're less likely to run into issues.