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.

Sign up for GitHub

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

Jump to bottom

Add Cloze #3793

Merged
merged 5 commits into from
Apr 30, 2019
Merged

Add Cloze #3793

merged 5 commits into from
Apr 30, 2019

Conversation

claudinec
Copy link
Contributor

Alexa global rank: 68,258.

kmpoppe
kmpoppe requested changes Apr 10, 2019
View reviewed changes
Copy link
Member

@kmpoppe kmpoppe left a comment

Choose a reason for hiding this comment

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

Hello @claudinec!
The image you supplied is larger than 2500 bytes, which is not allowed under the current rules. Please use https://tinypng.com/ to reduce the size of the image and change it in your branch by a new checkin.
Thanks for your contribution!

@Carlgo11 Carlgo11 added the add site Issue/PR adds a site to the repo. label Apr 10, 2019
@claudinec
Copy link
Contributor Author

Oops, missed that, try now?

kmpoppe
kmpoppe previously approved these changes Apr 11, 2019
View reviewed changes
Carlgo11
Carlgo11 requested changes Apr 11, 2019
View reviewed changes
Copy link
Member

@Carlgo11 Carlgo11 left a comment

Choose a reason for hiding this comment

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

You've defined them as supporting 2FA without any 2FA tags. Please add either software, hardware, email, sms or phone.

jtagcat
jtagcat suggested changes Apr 12, 2019
View reviewed changes
Copy link
Contributor

@jtagcat jtagcat left a comment

Choose a reason for hiding this comment

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

Cloze supports only sms.

_data/communication.yml Show resolved Hide resolved
@claudinec
Copy link
Contributor Author

Sorry, I should have checked this at the start.

The Cloze documentation doesn't match the reality - it offers both SMS and app-based authentication methods, as in the screenshot. I have emailed Cloze support asking them to update their documentation.

cloze-2019-04-16

@claudinec
Copy link
Contributor Author

Cloze have updated their documentation: https://help.cloze.com/knowledge_base/topics/how-do-i-enable-2-step-authentication

@kmpoppe kmpoppe dismissed their stale review April 16, 2019 16:53

Approval was premature

jtagcat
jtagcat previously approved these changes Apr 16, 2019
View reviewed changes
@kmpoppe kmpoppe requested a review from Carlgo11 April 28, 2019 10:54
@Carlgo11
Copy link
Member

@kmpoppe I can't approve the PR before the required changes have been implemented.

@kmpoppe
Copy link
Member

kmpoppe commented Apr 29, 2019

@Carlgo11 Sorry, I thought I had committed the changes, but I didn't ... 🤔
@claudinec Would you please incorporate my suggestion into your files?

@kmpoppe kmpoppe merged commit d1942bc into 2factorauth:master Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmpoppe kmpoppe kmpoppe approved these changes

@Carlgo11 Carlgo11 Carlgo11 approved these changes

@jtagcat jtagcat jtagcat left review comments

Assignees
No one assigned
Labels
add site Issue/PR adds a site to the repo.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@claudinec @Carlgo11 @kmpoppe @jtagcat