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

chore(doc): Fix variable issue in submit to showcase/starter docs #20373

Merged

Conversation

wwwebman
Copy link
Contributor

@wwwebman wwwebman commented Jan 1, 2020

Description

As contributor I would like to have a clear understanding of "submit site to showcase" contribution. The issue I see in this doc is a {titleofthesite}, {username} variables misunderstanding. Some of contributors threat it as a real variable which they can put to e.g. description section and it will be replaced on built time. But it does not work this way.

This fix should prevent contributors to use {titleofthesite} and {username} by making clear that these are not variables.

Documentation

https://www.gatsbyjs.org/contributing/site-showcase-submissions/
Related to: https://www.gatsbyjs.org/contributing/submit-to-starter-library/

Related Issues

Related to #20365
Related to #20332

@wwwebman wwwebman requested a review from a team as a code owner January 1, 2020 16:17
@DanielRuf
Copy link
Contributor

Probably duplicate of #20365

@wwwebman
Copy link
Contributor Author

wwwebman commented Jan 1, 2020

@DanielRuf , it is not a duplicate. Look at changes of #20365 : https://github.com/gatsbyjs/gatsby/pull/20365/files

@DanielRuf
Copy link
Contributor

It tries to solve the same issue. I saw the differences but we should close one of both.

@muescha
Copy link
Contributor

muescha commented Jan 2, 2020

they should both merged:
#20373 -> fixing the docs to have contribution in future without the wrong text
#20365 -> fixing in site.yml one result of the misreading doc

Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This is good to talk about so we can prevent confusion. I'm not sure removing the curly braces entirely is the right solution. I mentioned some other options and I'd be interested in thoughts.

@@ -50,7 +50,7 @@ Use the following template to ensure required fields are filled:
- title: (required)
url: (required)
main_url: (required)
source_url: (optional - https://github.com/{username}/{titleofthesite})
source_url: (optional - https://github.com/username/titleofthesite)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
source_url: (optional - https://github.com/username/titleofthesite)
source_url: (optional - https://github.com/{username}/{repository(_name)})

What do we think of something like this? Do you still have the same inclination about replacement?

I worry that if we drop all the curly braces it doesn't draw your attention to the changes you need to make in the template. Another option is <> but I'm not sure that would have solved the misunderstanding.

Copy link
Contributor Author

@wwwebman wwwebman Jan 3, 2020

Choose a reason for hiding this comment

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

I see your point. Let's maybe do it more KISS. I suggest revert back all changes besides line 31 => 7211473#diff-94bc22b42ee6b592b2c39e07adc7cbe0R31. To be honest, the only line 31 makes the biggest confusion. Otherwise, we might end up with something more complicated and not clear. @laurieontech wdty?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me. Would you mind making the update and then we can merge it in? Thanks for the discussion on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@wwwebman wwwebman force-pushed the fix/fix-submit-to-site-showcase-doc branch from 7211473 to 2519c17 Compare January 3, 2020 19:40
@laurieontech
Copy link
Contributor

Looks good. Thanks for the discussion and PR @wwwebman! Hopefully this will clear up that source of confusion for others.

@laurieontech laurieontech merged commit b2d2554 into gatsbyjs:master Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants