-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add page title service to generate pagetitle based on the breadcrumbs #139
Conversation
src/Service/PageTitle.php
Outdated
$breadcrumbs = $this->breadcrumbTrail->all(); | ||
$breadcrumbs =array_reverse($breadcrumbs); | ||
|
||
if (count($breadcrumbs) === 0) { |
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.
Why not empty($this->breadcrumbTrail->all())
in the beginning?
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.
fixed c789ac4
src/Service/PageTitle.php
Outdated
) { | ||
} | ||
|
||
public function getTitle(): string |
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.
How can I overrule the pagetitle? By setting it in the template and overruling {%block title %}
?
As there is a service, wouldn't it make more sense to have some method to set the title?
In my opinion the reversed breadcrumb is the fallback if no specific title is set. But I think it should work in the same way the breadcrumbs work. See https://github.com/sumocoders/FrameworkCoreBundle/blob/master/docs/development/breadcrumb.md. But with #[Pagetitle(XXX)]
? But maybe something we should discuss with other devs.
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.
It would be done by setting the {% block title %}, an attribute would work but seems a bit overkill.
@jonasdekeukelaere @davysumo @absumo What's your opinion
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.
{%block title %}
for now might be enough? Unless you think the reversed breadcrumb is not enough for a default? By looks of it, we didn't even set custom titles before?
PageTitle attribute will need some rework as you'll need a listener to set the page title based on the attribute, probably with an option to append or overwrite? Plus this PageTitle class probably will need a setter for the setting the title by the listener?
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.
By looks of it, we didn't even set custom titles before?
🙄
Adds a service to generate the page title based on the breadcrumbs and the fallback title