Skip to content

Conversation

@ManoelLobo
Copy link
Contributor

Regarding #8103

Starting the authentication tutorial. (Misc/Making a site with user authentication*)

Mostly setting intro and starting to work into content.

cc @shannonbux

@ManoelLobo ManoelLobo requested a review from a team October 17, 2018 11:36
@ManoelLobo ManoelLobo force-pushed the docs/authentication-tutorial branch from 0d9e8dc to c1db8eb Compare October 17, 2018 13:43
Added to expose to feedback
@ManoelLobo ManoelLobo force-pushed the docs/authentication-tutorial branch 2 times, most recently from 151d8a8 to f612e04 Compare October 17, 2018 18:17
Copy link
Contributor

@amberleyromo amberleyromo left a comment

Choose a reason for hiding this comment

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

I think this is looking pretty good! I would pull over some of the applicable details and links from this section. (Linking to the simple-auth site, the create-client-paths plugin, etc). We can remove that section from the overview page after that.

@shannonbux we'll need to rethink the content for this overview page once the content migration project is finished.

@pieh @DSchau any comments on the content of this page?

@shannonbux shannonbux mentioned this pull request Oct 18, 2018
51 tasks
@ManoelLobo
Copy link
Contributor Author

Hi, @amberleyromo. Thanks for the response.

I am writing in the style of a tutorial, mainly based on the simple-auth example availabe in the gatsby repo, but simplifying the code a bit (less routes and css styling). Added a few more steps for more feedback.

I added a section "Creating client-only routes" that can be expand to engulf more of the concepts presented in the current section avalaible in the page Building apps with Gatsby. If I understood correctly, that was the suggestion, right?

@amberleyromo
Copy link
Contributor

Hey @ManoelLobo -- the idea is to migrate content (#8103). The content is being pulled from a page that's a top-level overview page.

Each overview should give a short overview of its section of the guides and ideally fit “above the fold” (the reader can see the whole guide overview article on desktop without scrolling).

(From the Guide Overview style guide).

So I believe this content should be migrated over to this relevant doc, and once all relevant content is migrated out of that overview doc, it will be edited into a proper overview page.

Let me know if this answers your question?

@shannonbux if you have thoughts/want to chime in?

@ManoelLobo
Copy link
Contributor Author

ManoelLobo commented Oct 19, 2018

Ok, @amberleyromo, thanks for the clarification!

I noticed somethingthat may be causing confusion: there is a link under Guides section (Guides - Use cases with Gatsby (including apps) - Building a site with authentication) and another in Advanced Tutorials section (Advanced Tutorials - Making a site with user authentication). I started working on the latter, the tutorial one. Are these duplicates/should be merged/should have different contents?

- PrivateRoutes component
- Minor typos fixed
@ManoelLobo ManoelLobo force-pushed the docs/authentication-tutorial branch from 8532e96 to 948e06c Compare October 19, 2018 14:27
@ManoelLobo
Copy link
Contributor Author

I finished what I call "first complete draft", the main bulk of the tutorial should be here. Waiting for feedback & suggestions!

@shannonbux @amberleyromo

@amberleyromo
Copy link
Contributor

Hi @ManoelLobo -- I think my comment didn't get posted, sorry! I thought I replied to this:

I noticed somethingthat may be causing confusion: there is a link under Guides section (Guides - Use cases with Gatsby (including apps) - Building a site with authentication) and another in Advanced Tutorials section (Advanced Tutorials - Making a site with user authentication). I started working on the latter, the tutorial one. Are these duplicates/should be merged/should have different contents?

Very valid point. I will leave this up to @shannonbux for her word on the architecture / whether these were intended to serve two different purposes, or are duplicative.

Thanks for your continued work on this!

@ManoelLobo ManoelLobo force-pushed the docs/authentication-tutorial branch from 25b2215 to ddcd53c Compare October 22, 2018 18:10
Let me know if these don't reflect your original meaning! I just did some minor edits.
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.

@ManoelLobo this looks great from a style perspective! Excellent work on formatting the code snippets 👍

One style change needed before we can merge this: h1 is reserved for the title, so change any h1 headers to h2.

As far as the difference between the tutorial and doc go, they both should be different. The authentication doc should just explain that authentication is possible, explain how it is possible, and show a very short example, then link to example sites, this tutorial, and more information about client-only routes.

Want to write that doc too?

- Corrected section headings
- Cleaned code snippets
@ManoelLobo
Copy link
Contributor Author

Hey, @shannonbux, thanks for the review!

I changed the headers as you requested, and made small fixes that I found myself.

Want to write that doc too?

Sure! I should open a PR in the next hours. Should I ping you/mention any specific issue?

- Checking if the window global exists befor using it
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.

Ah, yes, one more request:

Add these example sites to the bottom of the tutorial under further reading:

Next, @DSchau will give this a technical review!

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.

This looks great. I left a few comments, most of them are mere nits rather than substantive!

I haven't seen the approach with PrivateRoute and the @reach/router Router component directly, but it seems to work fairly well.

I solved this problem with a custom Layout component that handles authentication (with React context), and whitelisted routes that don't require it, but I think both solutions work pretty well.

Thanks for the PR!

If you're interested, my approach is here:


<nav>
<Link to="/">Home</Link>
{` `}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: but these spaces should probably just be removed. I think it's an artifact from prettier, but a hard line break should work here instead of spacing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, they are there on purpose. The minification process removes any whitespace preceeded by a newline, so all the links stay side by side (<a>...</a><a>...</a>), and end up rendering as one big word (HomeProfileLogout) - the links would work normally, but visually they wolud look like one.

I could solve this with styling (inline styles or change the global css file), but thought it could be more code than needed to explain the tutorial (or maybe I'm a lazy typist 😆 )

Opinions?


<nav>
<Link to="/">Home</Link>
{` `}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here with the spaces!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same.

@ManoelLobo
Copy link
Contributor Author

@shannonbux links added 👍

@DSchau thanks for the review!
About your solution, it reminds me of some frameworks that set security/routes in config files, like PHP's Symfony. Pretty nice!

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.

Looks awesome!

@shannonbux shannonbux merged commit 23bda33 into gatsbyjs:master Oct 24, 2018
@gatsbot
Copy link

gatsbot bot commented Oct 24, 2018

Holy buckets, @ManoelLobo — 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!

jedrichards pushed a commit to jedrichards/gatsby that referenced this pull request Nov 1, 2018
Regarding gatsbyjs#8103 

Starting the authentication tutorial. (Misc/Making a site with user authentication*)

Mostly setting intro and starting to work into content.

cc @shannonbux
@ManoelLobo ManoelLobo deleted the docs/authentication-tutorial branch November 6, 2018 10:41
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
Regarding gatsbyjs#8103 

Starting the authentication tutorial. (Misc/Making a site with user authentication*)

Mostly setting intro and starting to work into content.

cc @shannonbux
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