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.

Sign up for GitHub

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

Jump to bottom

Fixing missing page titles #2914

Merged
merged 5 commits into from
Apr 2, 2019
Merged

Fixing missing page titles #2914

merged 5 commits into from
Apr 2, 2019

Conversation

youriwims
Copy link
Contributor

@youriwims youriwims commented Mar 28, 2019 •
edited
Loading

Closes #2367
Related PRs/issues #1262

View the app, to test.

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-2914 March 28, 2019 18:23 Inactive
@youriwims
Copy link
Contributor Author

youriwims commented Mar 28, 2019 •
edited
Loading

By updating base.html to use {{page.title}}, all seems to be fixed. However there's still this line in models. Do we want to remove that or rewrite it so that we're able to use page.meta_title instead?
cc: @Pomax @mmmavis

@youriwims youriwims requested review from mmmavis and Pomax March 29, 2019 19:04
@mmmavis
Copy link
Collaborator

mmmavis commented Mar 29, 2019

I don't know enough to confidently answer your q, @youriwims. @Pomax 👀 ?

@mmmavis mmmavis removed their request for review March 29, 2019 22:15
@Pomax
Copy link
Contributor

Pomax commented Apr 1, 2019 •
edited
Loading

We should use the meta title, if it exists. The title field is wagtail's basis for the page slug, and we want to be able to specify a more human+seo-friendly title through the SEO properties in the "promote" tab. Not all pages need that, though, so you probably want to turn this into if meta, use meta, else use title

Pomax
Pomax previously requested changes Apr 1, 2019
View reviewed changes
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

see comment

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-2914 April 1, 2019 21:18 Inactive
@youriwims youriwims requested a review from Pomax April 2, 2019 15:23
@youriwims
Copy link
Contributor Author

@Pomax ,worked with Gideon on this yesterday and the fix worked. Any clue why it may not be working now?

@Pomax
Copy link
Contributor

Pomax commented Apr 2, 2019

This seems to work fine, though, where are you seeing it not work?

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-2914 April 2, 2019 16:49 Inactive
@youriwims youriwims marked this pull request as ready for review April 2, 2019 16:56
@youriwims youriwims dismissed Pomax’s stale review April 2, 2019 16:58

meta_title wasn't showing on pages that extend base.html so we added the logic directly in base template

@youriwims youriwims temporarily deployed to foundation-mofostaging-pr-2914 April 2, 2019 16:58 Inactive
@youriwims
Copy link
Contributor Author

youriwims commented Apr 2, 2019 •
edited
Loading

Meta-title also appears on 404 page now as well. @Pomax

Screen Shot 2019-04-02 at 1 44 03 PM

Pomax
Pomax approved these changes Apr 2, 2019
View reviewed changes
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

muy bien, envíalo

@youriwims youriwims merged commit c275671 into master Apr 2, 2019
@youriwims youriwims deleted the donde-lapagina-titulo branch April 2, 2019 17:48
This was referenced Jun 3, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pomax Pomax Pomax approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@youriwims @mmmavis @Pomax @cadecairos