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

feat: pass MDX frontmatter title into page <title> #292

Merged
merged 5 commits into from
Aug 2, 2019
Merged

feat: pass MDX frontmatter title into page <title> #292

merged 5 commits into from
Aug 2, 2019

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Jul 31, 2019

this PR passes the MDX frontmatter title into the <Meta> component HTML title element. not sure how configurable we want this to be, but this is an option for getting dynamic page titles in the format we're looking for in the IDL site

@emyarod emyarod added this to the August 2019 milestone Jul 31, 2019
@vercel
Copy link

vercel bot commented Jul 31, 2019 •
edited
Loading

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://gatsby-theme-c-git-fork-emyarod-291-page-titles-not-usin-87b8d4.carbon-design-system.now.sh

@vercel vercel bot requested a deployment to staging July 31, 2019 13:35 Abandoned
@vercel
Copy link

vercel bot commented Jul 31, 2019

You are pushing commits at a very fast pace (accross the whole organization).
Due to that, we cannot deploy the commit 35a4536.

You can try again later or upgrade your plan.

@vpicone
Copy link
Contributor

vpicone commented Jul 31, 2019

@emyarod this looks great thanks. Sorry I was experimenting with the www site and might have caused the now deployment issue. Should be fixed with your next commit

Could you add optional description/keywords as well? I know brand center was asking for this.

@emyarod
Copy link
Member Author

emyarod commented Jul 31, 2019 •
edited
Loading

@vpicone just double checking, would optional description/keywords also be from MDX frontmatter? and would they be appended to the description/keywords from gatsby-config or would it be a replacement?

@vpicone
Copy link
Contributor

vpicone commented Jul 31, 2019

@emyarod yeah, that'd be ideal

@vpicone
Copy link
Contributor

vpicone commented Jul 31, 2019

If they don't specify in frontmatter, it needs to be undefined so as to not override the one from siteMetadata

@vpicone vpicone requested review from abbeyhrt and vpicone August 1, 2019 16:30
vpicone
vpicone suggested changes Aug 1, 2019
View reviewed changes
Copy link
Contributor

@vpicone vpicone left a comment

Choose a reason for hiding this comment

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

Could you add a Frontmatter section to the Markdown docs (preferably at the top). The Markdown page itself should make use of the frontmatter title/description/keyword features as well for testing and demonstration.

@emyarod emyarod force-pushed the 291-page-titles-not-using-mdx-frontmatter branch from 3de440f to 6d7af25 Compare August 1, 2019 18:50
@emyarod emyarod requested a review from vpicone August 1, 2019 18:51
@emyarod
Copy link
Member Author

emyarod commented Aug 1, 2019

local build was taking a while so I haven't had a chance to preview this but I wrote a few statements about the frontmatter. let me know if I should add more info @vpicone

vpicone
vpicone approved these changes Aug 2, 2019
View reviewed changes
Copy link
Contributor

@vpicone vpicone left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@emyarod I pushed a couple of updates, if you have a sec please review them

  1. The design language website uses the page title as it's title. I added a theme option to allow the developer to control the format of the title: site, page or append (title – pageTitle)
  2. Added your change to the Homepage template so it's title could be controlled as well.

@vpicone vpicone requested a review from alisonjoseph August 2, 2019 00:24
alisonjoseph
alisonjoseph reviewed Aug 2, 2019
View reviewed changes
packages/example/src/pages/guides/configuration.mdx Outdated
@@ -148,14 +148,16 @@ To add a link to the bottom of each page that points to the current page source

- `additionalFontWeights` – add support for additional Plex font weights. Don't forget to specify italics for the additional weights if needed.
- `mdxExtensions` – change the file extensions processed by `gatsby-mdx` (default ['.mdx', '.md'])
- `titleType` – the format of the title in your head. `site` for your site title, `page` for your individual page title, or `append` for "title – pageTitle" (default 'page')
Copy link
Member

Choose a reason for hiding this comment

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

I think we need some examples here to make it easier to understand what each one means.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alisonjoseph added

alisonjoseph
alisonjoseph reviewed Aug 2, 2019
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

This looks good, just want some more docs/examples so it is a little more obvious what each title type means/does.

@emyarod
Copy link
Member Author

emyarod commented Aug 2, 2019

@vpicone yeah looks good to me. wasn't sure what kind of flexibility we were gonna offer WRT page title but this gives users more options. what's causing the lockfile changes though?

@vpicone
Copy link
Contributor

vpicone commented Aug 2, 2019 •
edited
Loading

@emyarod mdx had a patch bump while I was working on it, nothin major. Could open a seperate PR if you’d prefer

@emyarod
Copy link
Member Author

emyarod commented Aug 2, 2019

@vpicone that's fine was just wondering what prompted the change

alisonjoseph
alisonjoseph approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Looks good! thanks

@vpicone vpicone merged commit 8c6bd01 into carbon-design-system:master Aug 2, 2019
@emyarod emyarod deleted the 291-page-titles-not-using-mdx-frontmatter branch August 2, 2019 17:22
This was referenced Aug 2, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vpicone vpicone vpicone approved these changes

@alisonjoseph alisonjoseph alisonjoseph approved these changes

@abbeyhrt abbeyhrt Awaiting requested review from abbeyhrt

Assignees
No one assigned
Labels
status: ready for review 👀
Projects
None yet
Milestone
August 2019
Development

Successfully merging this pull request may close these issues.
3 participants
@emyarod @vpicone @alisonjoseph