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: update tutorial to useStaticQuery #13449

Merged
merged 11 commits into from
Apr 25, 2019

Conversation

andrewfairlie
Copy link
Contributor

@andrewfairlie andrewfairlie commented Apr 18, 2019

Description

A colleague of mine came across this page when learning Gatsby and mentioned that the code looked very messy and a bit hacky.

I introduced him to the useStaticQuery hook which clicked a lot more for him. When I first started learning Gatsby I also found the current code sample using the component inelegant and I wonder how many people drop out of the tutorial at this point because of the perceived code-smell.

I don't know if it's the core team's intention for people to be using useStaticQuery over the original component but if they are, might I suggest we update the tutorial as this pull request suggests? I think it'd be more accessible and also introduces the idea of using hooks in a hopefully easy to understand way into the tutorial which I think is a useful addition.

@andrewfairlie andrewfairlie requested a review from a team April 18, 2019 12:47
@andrewfairlie
Copy link
Contributor Author

Sorry, I didn't run prettier on this. Will do now.

@sidharthachatterjee sidharthachatterjee added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Apr 18, 2019
@andrewfairlie
Copy link
Contributor Author

Upon @DSchau's suggestion I've removed reference to <StaticQuery> in favour of focusing just on useStaticQuery. I think this has helped focus the tutorial a lot.

@DSchau
Copy link
Contributor

DSchau commented Apr 18, 2019

@marcysutton what do you think about this?

Generally - I think we've given useStaticQuery enough time to "bake," and it's fully ready to be used. It seems simpler and more easily understandable than render props (StaticQuery), so I think I'm mostly in favor of the change--but as always, curious what you think!

DSchau
DSchau previously requested changes Apr 18, 2019
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.

Left some preliminary comments concerning correctness/syntax issues! Thanks!

docs/tutorial/part-four/index.md Outdated Show resolved Hide resolved
)

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.

Maybe move this up a line and then use the comment syntax? e.g.

// highlight-end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By having it on this line I was trying to highlight the introduction of return( which would be a new addition to a user following the tutorial

Copy link
Contributor Author

@andrewfairlie andrewfairlie Apr 19, 2019

Choose a reason for hiding this comment

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

In the latest version I kept the return statement in for the reasons in my previous comment, but I have adjusted it to t he comment syntax and seems to work. Could you clarify if that's correct though?

Builds and passes tests correctly locally so assuming it's a-ok.

</Link>
{children}
</div>
{/* 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.

What's the benefit of this? I think this should be removed (and the closing highlight-end tag too)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it shows the closing parenthesis from the return statement, which makes sense to me. I don't think the closing curly bracket needs to be in there, though.

Copy link
Contributor Author

@andrewfairlie andrewfairlie Apr 19, 2019

Choose a reason for hiding this comment

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

Because both are a change to what the user had so far in the tutorial I thought highlighting both would be valuable.

I worry that some readers might focus just on the highlighted changes and if we don’t highlight this change it’ll be a cause for confusion.

Thoughts?

docs/tutorial/part-four/index.md Outdated Show resolved Hide resolved
@marcysutton
Copy link
Contributor

marcysutton commented Apr 18, 2019

I agree that it's simpler, and this is a good change. I migrated a Create React App to Gatsby the other day and found hooks much easier to use than my earlier Gatsby projects! Thumbs up from me 👍

I think it confused matters by explaining quite a techy detail in this tutorial
@andrewfairlie andrewfairlie requested a review from DSchau April 19, 2019 11:00
@andrewfairlie
Copy link
Contributor Author

I've made some changes to the syntax highlighting and removed the paragraph explaining the use of return() because I think it made the tutorial lose its focus.

This is building correctly and passing tests locally, but I'm still unsure whether I've used the correct highlighting syntax.

In this version, I've continued to highlight the opening and closing of return() because I think it may be unfamiliar to the first time reader working through this tutorial and if we don't highlight it I think some may miss its inclusion and their code might not work leading to frustration.

@sidharthachatterjee
Copy link
Contributor

For visual context, this is what the highlight looks after this change:
Screenshot 2019-04-19 19 03 06

Don't mean to nit pick but I would remove the highlights on the return start and end

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Left some suggestions!

docs/tutorial/part-four/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-four/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-four/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-four/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-four/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-four/index.md Show resolved Hide resolved
docs/tutorial/part-four/index.md Show resolved Hide resolved
@sidharthachatterjee sidharthachatterjee changed the title tutorial: update to useStaticQuery docs: update tutorial to useStaticQuery Apr 25, 2019
@sidharthachatterjee sidharthachatterjee merged commit cb3f856 into master Apr 25, 2019
@sidharthachatterjee sidharthachatterjee deleted the andrewfairlie-patch-1-1 branch April 25, 2019 22:21
@andrewfairlie
Copy link
Contributor Author

@sidharthachatterjee sorry for slow response but thanks for helping :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants