-
Notifications
You must be signed in to change notification settings - Fork 274
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
feat(editlink): add to default template, style updates, associated docs #239
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://gatsby-theme-carbon-git-fork-tay1orjones-edit-link.carbon-design-system.now.sh |
Closes #162 |
Awesome!!! I'll dive more into this tomorrow, but looks solid at first glance. |
packages/gatsby-theme-carbon/src/components/EditLink/EditLink.module.scss
Show resolved
Hide resolved
packages/gatsby-theme-carbon/src/components/EditLink/EditLink.module.scss
Outdated
Show resolved
Hide resolved
@vpicone updated with requested changes |
@tay1orjones Great work. You nailed it! One last thing: you're correctly using the theme options object in the implementation, but in the docs you have them put the repository info into site metadata. It's fine if you want to throw it in the site metadata object now to make querying easier. I'd just switch the docs to pass the repository as a theme option: module.exports = {
__experimentalThemes: [
{
resolve: 'gatsby-theme-carbon',
options: {
repository: {
baseUrl:
'https://github.com/carbon-design-system/gatsby-theme-carbon',
subDirectory: '/packages/example',
},
},
},
],
}; |
@vpicone great point, thanks for the catch. I just pushed an update with that change. |
@tay1orjones aw dang just realized this doesn't work if the site is an EDIT: I'm working on this, I'll just push to your branch. |
For the // We need to provide the actual file that created a specific page to append links for EditLink.
// We can't do page queries from MDX templates, so we'll add the page's relative path to context after it's created.
// The context object **is** supplied to MDX templates through the pageContext prop.
exports.onCreatePage = ({ page, actions }) => {
const { createPage, deletePage } = actions;
const [relativePagePath] = page.componentPath.split('src/pages').splice('-1');
deletePage(page);
createPage({
...page,
context: {
...page.context,
relativePagePath,
},
});
}; |
|
||
const repository = 'https://github.com/temporary/master/tree'; | ||
const EditLink = ({ relativePagePath, repository: repositoryProp }) => { |
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.
Moved this to it's own query. Ideally data fetching would be separate from the component itself but we definitely don't want them sharing queries in case one gets shadowed.
Looks good to me, thanks! |
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.
This LGTM!
Just wanted to confirm this wouldn't add this link automatically to anyone upgrading correct? They would have to add the repository object to their config file first?
@alisonjoseph yeah it doesn't render unless a url is provided as a theme option. |
Just took a quick shot here, let me know your feedback and I can revise as necessary
baseUrl
andsubDirectory