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

readmeeeee #40

Merged
merged 10 commits into from
Apr 25, 2017
Merged

readmeeeee #40

merged 10 commits into from
Apr 25, 2017

Conversation

davemcorwin
Copy link
Contributor

Polish up the readme #22

README.md Outdated

### Devise
[Devise][devise] is an amazing gem! It is perfect when you want an all-in-one solution that handles user authentication and associated flows for your Rails/ERB app. Everything is in the box, including the routes, controllers, views, and even mailers to handle user auth. But we often use Rails as an API and/or wanted more control over all those pieces and it became difficult to peel back all the layers to just to confirm a users email.

Copy link
Contributor

Choose a reason for hiding this comment

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

a user's > I think we need the possessive apostrophe here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

README.md Outdated
See the [Api Docs][docs] for more details.

## Advanced
Sometimes in order to redeem a token, we want to make sure some additional information is present and possible save that to our model. For example, when implementing a password reset flow, we want to update the User with the new password and make sure that its valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly instead of possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link

@wade00 wade00 left a comment

Choose a reason for hiding this comment

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

I can definitely get by with this. I don't think I'd have any trouble getting set up.

The only feedback I have is that I stumbled over "tokenable" a few times. I'm used to hearing it around the office, but calling the action of confirming an account as a "tokenable" feels weird.

README.md Outdated
Does not take over your app, minimal magic, and only if you want it. Token Master works with your existing authentication solution.

### Flexible
Works for Apis, ERB apps and everything in between.
Copy link

Choose a reason for hiding this comment

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

APIs?

README.md Outdated

...
Lets revisit the Quick Start and fill in the details.
Copy link

Choose a reason for hiding this comment

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

I think it should be "Let's".

README.md Outdated

1. In Rails apps by default, the Token Master module is included in your `ApplicationRecord` base class. However, if necessary, you can add this yourself by include the following in your class:
Copy link

Choose a reason for hiding this comment

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

"by including the following".

@wade00
Copy link

wade00 commented Apr 25, 2017

@davemcorwin I "approved" the review even though I had a few comments. See above!

@davemcorwin
Copy link
Contributor Author

ty @wade00! I tried to introduce the concept of a tokenable but it probably needs more work

@inveterateliterate inveterateliterate merged commit 90f7d2b into master Apr 25, 2017
@inveterateliterate inveterateliterate deleted the readme-update branch April 25, 2017 21:45
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