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

Add index page title, add mechanism to clear title from defaultRoute. #2047

Merged
merged 11 commits into from
Jun 27, 2020

Conversation

askvortsov1
Copy link
Sponsor Member

Fixes #495

Changes proposed in this pull request:
Add index page title, add mechanism to clear title from defaultRoute.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

@franzliedke
Copy link
Contributor

Because we now auto-format our JS code with Prettier, this branch now has conflicts with master. Sorry about that. 😉

Please take the steps outlined in the forum to resolve the conflicts.

@askvortsov1 askvortsov1 force-pushed the as/frontend_title_improvements branch from ca8d5bb to 7d6dcb0 Compare April 21, 2020 15:06
@clarkwinkelmann
Copy link
Member

I believe this PR only changes the behavior on the javascript side, the title on the PHP/preload side will still include the page title?

This does not need to be the same PR, but changing one and not the other wouldn't make sense.

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

@askvortsov1 what's the plan regarding to my comments above?

Also other comments:

  • Should use triple ===
  • I think the title count feature should also work on the homepage

@askvortsov1 askvortsov1 force-pushed the as/frontend_title_improvements branch from cb88ede to 0b486d2 Compare May 24, 2020 02:22
@askvortsov1
Copy link
Sponsor Member Author

Done, all good points. Thanks!

Also, now that Document has access to request, perhaps we should just use the current url for canonical url (in a separate PR)? That's often what we do anyway...

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

Looking better! Just one question in the comments below.

Also, now that Document has access to request, perhaps we should just use the current url for canonical url (in a separate PR)? That's often what we do anyway...

Definitely something for another issue/PR. I am doubtful that's how we do it.

src/Frontend/Document.php Outdated Show resolved Hide resolved
Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

Yep, I definitely think it's cleaner that way, even though both options would do practically the same.

That way people reading the code don't need to dig into the documentation to find out what originalUri means.

@askvortsov1 askvortsov1 merged commit a33fbbf into flarum:master Jun 27, 2020
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.

Add titles for pages that lack them
5 participants