Skip to content

Conversation

@bjorvack
Copy link
Contributor

Adds a service to generate the page title based on the breadcrumbs and the fallback title

$breadcrumbs = $this->breadcrumbTrail->all();
$breadcrumbs =array_reverse($breadcrumbs);

if (count($breadcrumbs) === 0) {
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed c789ac4

) {
}

public function getTitle(): string
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Member

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?
🙄

@bjorvack bjorvack merged commit 638f717 into master Jan 22, 2024
@jonasdekeukelaere jonasdekeukelaere deleted the page-title-service branch October 11, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants