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 Authentication doc to cover all backends #751

Merged
merged 3 commits into from
Nov 27, 2017
Merged

Conversation

verythorough
Copy link
Contributor

@verythorough verythorough commented Oct 27, 2017

Note that this docs update (or any other docs edit) will not display on netlifycms.org until #749 is resolved. Resolved with 83a1d34

- Summary
As described in #600, this adapts the existing custom-authentication.md to a more general authentication-backends.md, with instructions and reference material for all existing backend options.

I'd really love feedback on this. I struggled with explaining concepts and contraints without creating huge blocks of text people would skip over. I'm uncertain about the "More config options" section at the bottom, particularly because a few of those are github-specific. (Will they apply to gitlab and bitbucket?) Also, unlike the others options, branch seems common enough that it might be worth putting in the config samples for both backends.

- Test plan
I generated a preview you can check out, but had to break the Extending Widgets doc in that branch so the preview build would compete. As mentioned above, #749 will have to be resolved before any docs edits appear on netlifycms.org. (resolved)

- Description for the changelog
Updated Authentication & Backends documentation

- A picture of a cute animal (not mandatory but encouraged)
For Halloween: a pure bread shiba
Shiba dressed as a loaf of bread

@erquhart
Copy link
Contributor

erquhart commented Nov 7, 2017

@verythorough I reviewed your PR by opening a PR against your PR. You can comment on my PR about your PR on my PR, or right here on your PR, or by opening another PR against my PR against your PR. Or whatever :) Love the bread-dog!

Seriously though, we'll continue discussion here. Preview link: https://preview-auth-doc-update--netlify-cms-www.netlify.com/docs/authentication-backends/

@verythorough
Copy link
Contributor Author

lol, thanks. Not sure why I had such a block with this one, but your edits help a lot. I like the numbered steps. I was this close to using the table format for the options, but the layout makes them a bit hard to read. I may futz with the styling a bit to make that better, because I think we'd benefit from more tables throughout the docs.

I'm going to merge your PR into this one and comment/edit in here from that point.

@erquhart
Copy link
Contributor

erquhart commented Nov 7, 2017

I think the table readability issue is just due to our content column being so narrow. We should spread the max by a good 25% at least.

@verythorough
Copy link
Contributor Author

Long line lengths hurt readability. Recommendations for ideal line length vary, but are generally below 85 characters per line, and typically 75 or lower. The current template has max line lengths in the 90s, so I wouldn't want to widen it much, if at all.

@Benaiah
Copy link
Contributor

Benaiah commented Nov 8, 2017

I wonder if it's possible to let the table flow to a bigger max-width than the body text. Haven't worked on the CMS site CSS yet, but I know I've done similar things on sites I've done to help relieve that conflict.

@erquhart
Copy link
Contributor

erquhart commented Nov 8, 2017

We could make text a little bigger. Again, drawing inspiration from reactjs.org/docs. (and maybe table text a little smaller)

@verythorough
Copy link
Contributor Author

The React docs appear to be using 42em content width with a 17px body font (not counting the lede at the beginning of the page), while netlifycms.org is 45em and 18px, so they're kinda the same size already.

The table text is already significantly smaller than the body font, so I'm thinking something more along the lines of reducing the table cell padding. Just removing the 24px side padding produces a significantly more readable result:
screen shot 2017-11-08 at 11 01 25 pm
vs.
screen shot 2017-11-08 at 11 03 23 pm

I plan to do a little more than that, though, to keep it clear and pretty.

Copy link
Contributor Author

@verythorough verythorough left a comment

Choose a reason for hiding this comment

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

I made some edits to the doc, and now consider it ready to merge, pending review.
I've added line comments to explain some of the changes.

The preview has updated to include the changes: https://preview-auth-doc--netlify-cms-www.netlify.com/docs/authentication-backends/

@flaturtha, @eamonnbell Would you like to take a look?

Netlify CMS stores content in your GitHub repository (GitLab and Bitbucket coming soon!). You'll
need to sign in to your GitHub account through the CMS for this to work, and it requires a server.
We have a few options for handling this.
Netlify CMS stores content in your GitHub repository. (GitLab and Bitbucket coming soon!) In order for this to work, you need to authenticate with GitHub, and that requires a server. We have a few options for handling 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.

I liked "You need to sign in to your GitHub account," but it doesn't actually apply to Git Gateway. "Authenticate" is more formal, but also more accurate.


Using it in your own project is simple:
To use it in your own project, follow these steps:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I try to avoid using words like "simple," "easy," or "just do x" in docs, since the reader may disagree and/or feel judged.

name: git-gateway
accept_roles: #optional - accepts all users if left out
- admin
- editor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Matched the format to our other yaml code snippets

backend:
name: github
repo: owner-name/repo-name # Path to your Github repository
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to indent the code blocks one more tab to make them properly display in the numbered list, here and in the Git Gateway steps.
(Side note: we should take a look at how the rich text editor marks up code blocks inside of lists, and be sure they produce the desired output in most cases.)

request](https://github.com/netlify/netlify-cms/blob/master/CONTRIBUTING.md) if you'd like to add
yours!
can use one of the community-maintained projects below. Feel free to [submit a pull request](https://github.com/netlify/netlify-cms/blob/master/CONTRIBUTING.md) if you'd like to add yours!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On re-reading, this paragraph made more sense under the External OAuth Clients heading than before it.

|----------------|-------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------|
| `repo` | none | **Required** for `github` backend; ignored by `git-gateway`. Follows the pattern `[org-or-username]/[repo-name]`. |
| `accept_roles` | none | `git-gateway` only. Limits CMS access to your defined array of user roles. Omitting this field gives access to all registered users. |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the "Required" column because it took up a lot of space, and as it turns out, none of them are required in all cases. Instead, I added "Required for x" in the description.
I also added accept_roles so we now cover all possible fields (except name which felt kind of weird to include—open to adding, though).

@verythorough verythorough changed the title [WIP] Update Authentication doc to cover all backends Update Authentication doc to cover all backends Nov 14, 2017
@verythorough
Copy link
Contributor Author

The preview also now includes style changes from netlify/netlify-cms-www#53 (but the preview in that PR does not include the changes from this one 😝 )

@verythorough
Copy link
Contributor Author

I think this is ready to merge. Once merged, we'll want to check the preview for netlify/netlify-cms-www#57, and then merge that to get the changes applied to netlifycms.org.

@erquhart erquhart closed this Nov 27, 2017
@erquhart erquhart deleted the docs-authentication branch November 27, 2017 18:54
@erquhart erquhart restored the docs-authentication branch November 27, 2017 19:20
@erquhart erquhart reopened this Nov 27, 2017
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.

3 participants