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

Prefer site.tagline to site.description for page title #356

Merged
merged 6 commits into from Nov 29, 2019

Conversation

qwtel
Copy link
Contributor

@qwtel qwtel commented Aug 18, 2019

Using site.description as part of the title for pages that don't have an explicit title --- typically the landing page of a site --- leads to very long page titles. The site.description field is typically used for search engine or social media previews, which are 100+ characters long, making it a bad choice for the title for pages that use the description field in this way.

This PR changes the behavior of this plugin to prefer site.tagline over site.description when it is available. Sites that do not define a site.tagline are not affected by this.

The name site.tagline was chosen as it served the same role in the Hyde theme.

@ashmaroli
Copy link
Member

@qwtel Can you add a test for a scenario that doesn't have site.tagline defined? Thanks.

@qwtel
Copy link
Contributor Author

qwtel commented Aug 19, 2019

I didn't add one specifically b/c site.tagline being undefined is the case for the entire current test suite

@qwtel
Copy link
Contributor Author

qwtel commented Nov 10, 2019

Can this be merged now?

@ashmaroli ashmaroli requested a review from a team November 10, 2019 11:50
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Seems fine to me. It needs documentation.

# Page title with site title or description appended
# rubocop:disable Metrics/CyclomaticComplexity
def title
@title ||= begin
if site_title && page_title != site_title
page_title + TITLE_SEPARATOR + site_title
elsif site_description && site_title
site_title + TITLE_SEPARATOR + site_description
site_title + TITLE_SEPARATOR + site_tagline_or_description
Copy link
Member

Choose a reason for hiding this comment

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

If this is the only place site_tagline_or_description is used, I’d suggest inlining the method. Feel free to use parentheses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to do that, but it created a style offence:

  • Metrics/AbcSize: Assignment Branch Condition size for title is too high
  • Metrics/PerceivedComplexity: Perceived complexity for title is too high

@qwtel

This comment has been minimized.

@qwtel
Copy link
Contributor Author

qwtel commented Nov 13, 2019

I've updated the documentation, how does it look?

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @qwtel

@DirtyF DirtyF added this to the Next minor version milestone Nov 13, 2019
@qwtel
Copy link
Contributor Author

qwtel commented Nov 29, 2019

Can this be merged now? 😉

@DirtyF
Copy link
Member

DirtyF commented Nov 29, 2019

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 3319035 into jekyll:master Nov 29, 2019
jekyllbot added a commit that referenced this pull request Nov 29, 2019
@jekyll jekyll locked and limited conversation to collaborators Nov 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants