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

Update tutorial one to reference gatsby astronaut image #9698

Closed
wants to merge 3 commits into from
Closed

Update tutorial one to reference gatsby astronaut image #9698

wants to merge 3 commits into from

Conversation

ebello
Copy link

@ebello ebello commented Nov 4, 2018

This will close #9655 but should not be merged until gatsby-starter-hello-world adds the referenced image, which @barrymcgee did in gatsbyjs/gatsby-starter-hello-world#27.

@ebello ebello requested a review from a team November 4, 2018 22:29
@barrymcgee barrymcgee self-requested a review November 5, 2018 12:30
@barrymcgee barrymcgee added do not merge status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged and removed do not merge labels Nov 5, 2018
Copy link
Contributor

@barrymcgee barrymcgee left a comment

Choose a reason for hiding this comment

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

Sorry, I took another look at my PR this morning and realised the most appropriate place to add the astronaut image is in the static folder.

When a user then runs npm start, this will be copied to the public folder and available to link in the src attribute as simply ./images/gatsby-astronaut.png.

I believe importing the image here as you have (I understand you took this approach based on my previous proposal) is the wrong approach to suggest to new users as they may then try to scale this approach up for every image they want to add to a page.

Would you mind updating the code snippet and screenshot to reflect my updated changes?


```jsx{7}:title=src/pages/index.js
```jsx{2,8}:title=src/pages/index.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain to me what this change is please? I haven't seen it before...

Copy link
Author

Choose a reason for hiding this comment

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

This is code snippet line highlighting. It specifies that line 2 and 8 should be highlighted to denote the changes made.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL - thank you.

@ebello
Copy link
Author

ebello commented Nov 6, 2018

Not a problem, this is even easier which is the spirit of the tutorial. Just pushed.

@barrymcgee
Copy link
Contributor

@ebello Looks good to me - once my PR to add the image to the starter lands, this can land too 👌🏻

Copy link
Contributor

@shannonbux shannonbux left a comment

Choose a reason for hiding this comment

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

Just two pieces of feedback here:

  1. Where would the image be getting pulled from? I looked in https://github.com/gatsbyjs/gatsby-starter-hello-world and can't find it.

  2. I love this image and think it'd be fun to introduce it at some point. However, if tutorial users don't know how to replicate this step of adding an image, I wonder if it's really worth introducing this step, since the purpose of the tutorial is to teach people how to build a site. I think adding an image tutorial that teaches people how to use gatsby-image would be the most valuable thing to do, and it could use this image.

@ebello
Copy link
Author

ebello commented Nov 6, 2018

Hi @shannonbux, we have an outstanding PR from @barrymcgee to add the image to the starter here:
gatsbyjs/gatsby-starter-hello-world#27.

I like your idea of using the image with gatsby-image, though I think that might be a more advanced step. Introducing the static folder here as a place to put assets like the image might be a more useful building block at this stage. Perhaps we could add a bit of text in the tutorial that introduces the notion of optimizing images and link people off to the "Using gatsby-image" guide?

@shannonbux
Copy link
Contributor

Hello everyone! Thanks for your hard work here and apologies for the time it took to reach a decision on this PR and the connected one. For now, I'm going to close this in favor of #9374 since a adding a page to the tutorial specifically on gatsby-image introduces a best practice. Thanks in advance for your patience with this idea; we only recently realized how few people know about gatsby-image and we'd like to use the tutorial as one of the ways to teach about it.

@shannonbux shannonbux closed this Nov 12, 2018
@shannonbux
Copy link
Contributor

It will be valuable to get your input on that new gatsby-image tutorial and honestly, if any of you feels up to reviewing all the image-related docs and cleaning them up, they could use it!

Here's the issue for cleaning those up!
#4319

@barrymcgee
Copy link
Contributor

Hi @shannonbux - thanks for bringing this to a conclusion. Just so I know for future, what's the process around approving issues so we know that when we start an issue, it's a valid/approved issue?

I, and I'm sure @ebello, would like to avoid this situation in future where we spent time working on an issue that it seems no-one agreed it should have been worked on at all.

If I get some time this week, I'll have a look at the gatsy-image tasks.

@shannonbux
Copy link
Contributor

shannonbux commented Nov 13, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update tutorial one to use the cute gatsby illustration
3 participants