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

docs: migrated line highlighting to highlight-line, highlight-start, highlight-end #10202

Merged
merged 6 commits into from
Dec 7, 2018

Conversation

nero2009
Copy link
Contributor

This Pull request closes #10134

@nero2009 nero2009 requested a review from a team November 29, 2018 16:04
@nero2009
Copy link
Contributor Author

Side note: highlight-line doesn't work for html or jsx had to use highlight-next-line in the preceding line to highlight that line.

@pieh pieh self-assigned this Nov 29, 2018
@DSchau
Copy link
Contributor

DSchau commented Nov 29, 2018

@nero2009 could you provide some more information on this comment?

Side note: highlight-line doesn't work for html or jsx had to use highlight-next-line in the preceding line to highlight that line.

It appears like it does in my testing! Check out this example for an illustration of it working.

@nero2009
Copy link
Contributor Author

@nero2009 could you provide some more information on this comment?

Side note: highlight-line doesn't work for html or jsx had to use highlight-next-line in the preceding line to highlight that line.

It appears like it does in my testing! Check out this example for an illustration of it working.

My bad, didn't use the jsx style for commenting code. used the double slash //

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Looking great! I'll do a more thorough review once the JSX comments are addressed, but this is an awesome start.

Thanks so much!

<li
key={pokemon.id}
key={pokemon.id} // highlight-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Be careful of JSX here. We want these snippets to remain valid JSX, so it'll end up looking something more like this:

```jsx:title=src/templates/all-pokemon.js
export default ({ pageContext: { allPokemon } }) => ( // highlight-line
    {...}
        {allPokemon.map(pokemon => ( // highlight-line
            <li
                key={pokemon.id} // highlight-line
                style={{
                    textAlign: 'center',
                    listStyle: 'none',
                    display: 'inline-block'
                }}
            >
            {/* highlight-start */}
                <Link to={`/pokemon/${pokemon.name}`}>
                    <img src={pokemon.sprites.front_default} alt={pokemon.name} />
                    <p>{pokemon.name}</p>
            {/* highlight-end */}
                </Link>
            </li>
        ))}
    {...}
);
```

@@ -105,21 +109,23 @@ The [`createPage` action](/docs/actions/#createPage) is passed an object contain

In our example, we're accessing the context as [props to the component](https://github.com/jlengstorf/gatsby-with-unstructured-data/blob/0a91d87b9d4d24a0e6b04b33cc271e054b7467b6/src/templates/all-pokemon.js#L4). This allows us to completely circumvent Gatsby’s data layer; it’s just props.

```jsx{1,3,5,12-14}:title=src/templates/all-pokemon.js
export default ({ pageContext: { allPokemon } }) => (
```jsx:title=src/template/all-pokemon.js
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
```jsx:title=src/template/all-pokemon.js
```jsx:title=src/templates/all-pokemon.js

<Helmet>
<meta charSet="utf-8" />
<title>My Title</title>
<link rel="canonical" href="http://mysite.com/example" />
</Helmet>
...
//highlight-end ...
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
//highlight-end ...
//highlight-end

@@ -103,6 +103,7 @@ const Layout = ({ children }) => (
paddingTop: 0,
}}
>
// highlight-next-line
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
// highlight-next-line
{/* highlight-next-line */}

(I'm not going to comment on all of these, so if you're able, another look at the JSX snippets would probably be a good idea!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'd do that

Copy link
Contributor

Choose a reason for hiding this comment

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

If it helps, using VSCode (and possibly extensions in other editors) is really helpful as it will syntax highlight the snippets. So it's clear at a glance if a snippet is broken!

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops - apparently not. Just need to spot check them!

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

I've started testing (so there are some valid inline comments from me), but I think first we need to sort out jsx comments, before doing full review, but this is awesome - is so much nicer to handle those inline comments for highlighting than ranges

exports.createPages = () => {
// highlight-line
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be comment in line above:

exports.createPages = () => { // highlight-line

exports.createPages = async ({ actions: { createPage } }) => {
//highlight-line
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be comment in line above:

exports.createPages = async ({ actions: { createPage } }) => { // highlight-line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prettier moves it to the next line, when I make a commit.
#10202 (comment)

@@ -103,6 +103,7 @@ const Layout = ({ children }) => (
paddingTop: 0,
}}
>
// highlight-next-line
<NavBar />
Copy link
Contributor

Choose a reason for hiding this comment

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

As @DSchau mentioned - please use:

<NavBar /> {/* highlight-line */}

Right now this is invalid JSX, so if people copy this from source or would be reading that on github they could copy not working code

@@ -129,9 +130,11 @@ import Layout from "../components/layout"
const IndexPage = () => (
<Layout>
<h1>Hi people</h1>
// highlight-start
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
// highlight-start
{/* highlight-start */}

<p>
You should <Link to="/">log in</Link> to see restricted content
</p>
// highlight-end
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
// highlight-end
{/* highlight-end */}

@@ -367,6 +372,7 @@ export default () => {
content.message = "You are not logged in"
}
return (
// highlight-end
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
// highlight-end
{/* highlight-end */}

@nero2009
Copy link
Contributor Author

nero2009 commented Nov 30, 2018

In the pre-commit hook prettier changes

exports.createPages = () => { // highlight-line

  // Run this code
} 

to

exports.createPages = () => {
  // highlight-line
  // Run this code
} 

how can i get prettier to ignore it.

@pieh
Copy link
Contributor

pieh commented Nov 30, 2018

Ahhhh, it was prettier that messed some things up :/ I think we would need to either let prettier know to ignore highlight comments (if possible) or use highlight-next-line as workaround :/

@nero2009
Copy link
Contributor Author

Ahhhh, it was prettier that messed some things up :/ I think we would need to either let prettier know to ignore highlight comments (if possible) or use highlight-next-line as workaround :/

Ok I will use highlight-next-line

nero-adaware and others added 3 commits November 30, 2018 14:31
@phacks
Copy link
Contributor

phacks commented Nov 30, 2018

Sooo happy to see this! Thanks @nero2009, awesome work 🎉

@nero2009
Copy link
Contributor Author

nero2009 commented Dec 7, 2018

Hi @DSchau are there any problems with this PR so that I can resolve them over the weekend?
And I also noticed that the circleci check failed for my last two commits,Please how can I resolve that?

@DSchau
Copy link
Contributor

DSchau commented Dec 7, 2018

@nero2009 looks like we have some merge conflicts, but presuming the suggestions were fixed, we can get this merged today! I think it's a big improvement 🎉

@DSchau DSchau changed the title migrated line highlighting to highlight-line, highlight-start, highlight-end docs: migrated line highlighting to highlight-line, highlight-start, highlight-end Dec 7, 2018
@DSchau DSchau merged commit 43fbcbe into gatsbyjs:master Dec 7, 2018
@gatsbot
Copy link

gatsbot bot commented Dec 7, 2018

Holy buckets, @nero2009 — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

m-allanson added a commit that referenced this pull request Dec 8, 2018
* master: (1421 commits)
  feat(gatsby-image): add onStartLoad prop  (#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (#10350)
  fix(www): Starters.yaml housekeeping (#10354)
  docs: add ttag starter (#10352)
  docs: document branching (#9983)
  plugin checker initial commit (#7062)
  docs: new starter features is required (#10353)
  docs: migrated line highlighting to highlight-line, highlight-start, highlight-end (#10202)
  Add Birra Napoli to site showcase (#10344)
  Add BetterDocs to site showcase (#10349)
  chore(release): Publish
  Add option to keep metadata in files processed by `gatsby-plugin-sharp` (#10210)
  fix(gatsby): [loki] sync db autosaves (#10212)
  Add Ad Hoc Homework to sites.yml (#10346)
  fix(graphql-skip-limit): declare `graphql` peer dependency (#10305)
  fix(gatsby-plugin-offline): gracefully degrade if appshell isn't precached (#10329)
  Service workers note (#10276)
  fix(docs): link fixes, podcast addition (#10332)
  feat(docs): Create clearer pathways in docs (#9898)
  feat(www): Rename community section to creators (#10312)
  ...
m-allanson added a commit to Bouncey/gatsby that referenced this pull request Dec 10, 2018
* master: (35 commits)
  feat(gatsby-source-filesystem): keep original name of remote files (gatsbyjs#9777)
  docs(gatsby-source-contentful): Rewrite of gatsby-source-contentful query section (gatsbyjs#7533)
  Update "Deploying with Now" Guide (gatsbyjs#10390)
  Add Matomo to list of analytics plugins (gatsbyjs#10372)
  Add satispay.com to showcase (gatsbyjs#10380)
  Adds @goblindegook/gatsby-starter-typescript (gatsbyjs#10377)
  Fix typo in gatsby-remark-code-repls sample `gatsby-config.json` in README (gatsbyjs#10361)
  fix(gatsby): fix false type conflict warning on plugin fields (gatsbyjs#10355)
  fix(docs): Just a small typo fix in the docs (gatsbyjs#10359)
  feat(gatsby-image): add onStartLoad prop  (gatsbyjs#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (gatsbyjs#10350)
  fix(www): Starters.yaml housekeeping (gatsbyjs#10354)
  docs: add ttag starter (gatsbyjs#10352)
  docs: document branching (gatsbyjs#9983)
  plugin checker initial commit (gatsbyjs#7062)
  docs: new starter features is required (gatsbyjs#10353)
  docs: migrated line highlighting to highlight-line, highlight-start, highlight-end (gatsbyjs#10202)
  Add Birra Napoli to site showcase (gatsbyjs#10344)
  Add BetterDocs to site showcase (gatsbyjs#10349)
  chore(release): Publish
  ...
m-allanson added a commit to markza/gatsby that referenced this pull request Dec 11, 2018
* master: (1432 commits)
  chore(release): Publish
  Fix Starter Library URL and update copy (gatsbyjs#10368)
  feat(gatsby-source-filesystem): keep original name of remote files (gatsbyjs#9777)
  docs(gatsby-source-contentful): Rewrite of gatsby-source-contentful query section (gatsbyjs#7533)
  Update "Deploying with Now" Guide (gatsbyjs#10390)
  Add Matomo to list of analytics plugins (gatsbyjs#10372)
  Add satispay.com to showcase (gatsbyjs#10380)
  Adds @goblindegook/gatsby-starter-typescript (gatsbyjs#10377)
  Fix typo in gatsby-remark-code-repls sample `gatsby-config.json` in README (gatsbyjs#10361)
  fix(gatsby): fix false type conflict warning on plugin fields (gatsbyjs#10355)
  fix(docs): Just a small typo fix in the docs (gatsbyjs#10359)
  feat(gatsby-image): add onStartLoad prop  (gatsbyjs#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (gatsbyjs#10350)
  fix(www): Starters.yaml housekeeping (gatsbyjs#10354)
  docs: add ttag starter (gatsbyjs#10352)
  docs: document branching (gatsbyjs#9983)
  plugin checker initial commit (gatsbyjs#7062)
  docs: new starter features is required (gatsbyjs#10353)
  docs: migrated line highlighting to highlight-line, highlight-start, highlight-end (gatsbyjs#10202)
  Add Birra Napoli to site showcase (gatsbyjs#10344)
  ...
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…highlight-end (gatsbyjs#10202)

* migrated line highlighting to highlight-line, highlight-start, highlight-end

* Changed highlighting to use valid commenting syntax

* Minor changes

* Update authenication-tutorial.md

Updated this file to workaround changes made by prettier.

* Update using-unstructured-data.md
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.

docs: migrate line highlighting to highlight-line, highlight-start, highlight-end
5 participants