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

Reduce conditionals in signin/signup inner forms #1138

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Reduce conditionals in signin/signup inner forms #1138

merged 1 commit into from
Mar 21, 2017

Conversation

strk
Copy link
Member

@strk strk commented Mar 7, 2017

by always using SignInLink and SignUpLink in the form action

@lunny lunny added this to the 1.2.0 milestone Mar 7, 2017
@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 7, 2017
@lunny
Copy link
Member

lunny commented Mar 10, 2017

why not use LinkAccountMode?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 10, 2017
@strk
Copy link
Member Author

strk commented Mar 11, 2017

The goal of this PR is to reduce conditionals, so the reason NOT to use LinkAccountMode is because it'd be another conditional. Why would you want to add so much logic in a template ?

Templates would ideally be as simple as possible, as they are meant to be customized.

Now why would you keep it ?
Also, what does LinkAccountMode mean, exactly ? What are you linking ? Where does it say it's about OAuth2 ? Or should it also be for OpenID ? Let's start reducing the use of those conditionals, then maybe we'll be able to do w/out them completely.

@strk strk force-pushed the simplify_auth_templates branch 2 times, most recently from f678a3d to 6a14ec7 Compare March 16, 2017 07:25
@strk
Reduce conditionals in signin/signup inner forms
15f5d8e 
by always using SignInLink and SignUpLink in the form action
@ethantkoenig
Copy link
Member

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 21, 2017
@appleboy
Copy link
Member

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 21, 2017
@bkcsoft
Copy link
Member

bkcsoft commented Mar 21, 2017

It's better than before so I'm merging :)

@bkcsoft bkcsoft merged commit c05bd17 into go-gitea:master Mar 21, 2017
@strk strk deleted the simplify_auth_templates branch April 10, 2017 19:48
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@appleboy appleboy appleboy approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
6 participants
@strk @lunny @ethantkoenig @appleboy @bkcsoft @tboerger