-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
docs: add a title to code snippets in the first part of the tutorial #7867
Conversation
@@ -206,17 +206,17 @@ export default () => ( | |||
|
|||
In the browser, the “About Gatsby” header text should now be replaced with “This is a header.” But we don’t want the “About” page to say “This is a header.” We want it to say, “About Gatsby”. | |||
|
|||
4. Head back to `/src/components/header.js`, and make the following change: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed these from absolute paths to relative. Good idea or bad idea? If nothing else, we should be consistent throughout the docs and throughout the tutorial.
borderTopLeftRadius: 8, | ||
borderTopRightRadius: 8, | ||
color: `white`, | ||
fontFamily: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also get our font families from
gatsby/www/src/utils/typography.js
Line 30 in c87e937
monospaceFontFamily: [ |
typography.options.monospaceFontFamily.join(",")
)}`, | ||
}, | ||
}, | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be abstracted so codeblock title
could be used in blog posts as well? Not high priority, but would be nice if it's easy to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I should probably just add it in the src/util/typography file instead of at the component level! I'll do that tonight probably.
@pieh this is ready if you wanna take another gander 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Let's merge this in, we can always iterate on it later on!
Addresses #5608
Note: there is more to do here, i.e. actually copy these titles over to relevant tutorial pages, but I think this is a start.
Here's what it ends up looking like.
Will want to fix/externalize the padding/media query stuff. That feels a little janky.