-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Simple signup #2881
Simple signup #2881
Conversation
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me> cleanup Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Codecov Report
@@ Coverage Diff @@
## master #2881 +/- ##
=========================================
- Coverage 6.45% 6.44% -0.02%
=========================================
Files 296 300 +4
Lines 29624 29663 +39
Branches 4282 4285 +3
=========================================
- Hits 1913 1912 -1
- Misses 27426 27466 +40
Partials 285 285
|
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
11e468b
to
b24f6c2
Compare
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
createAccount(false); | ||
Intent firstRunIntent = new Intent(getApplicationContext(), FirstRunActivity.class); | ||
firstRunIntent.putExtra(FirstRunActivity.EXTRA_ALLOW_CLOSE, true); | ||
startActivity(firstRunIntent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want to run the "FirstRunActivity" every time a user wants to add another account? I am fine with having this as a very first run whenever the app is installed (not updated) but even during testing this annoyed my since I always have to click login to get to the real screen since all of my accounts are on the same server. And even if not, why would I want to see the wizard again and not just type in the URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you propose to show this only when creating the very first account?
But what would be if you want to create a second account on a different provider?
That is why I chose to show it every time, but I am up for better ideas :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also interested in @jancborchardt's opinion UX wise since it leads to one additional click every time someone wants to add another account :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @AndyScherzinger, but I would say we should indeed always show this first run activity. There are lots of cases which lead you to adding another account:
- One of them is having another account already set up
- Another is wanting to create a completely new one
- Another is wanting to create one and then being made aware of the ability to set up your own server.
- Another is just to see what happens when you tap it, to find out about what the feature means.
Especially as adding another account will not happen that often, the additional click in one of the cases is not that bad. Does that sound ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also fine with this. We release 3.2.2 this week and can see what the users feedback is.
And then, maybe change it for 3.3.0.
Hope you are fine with this approach, @AndyScherzinger?
👍 @tobiasKaminsky all good and working except for one comment, see #2881 (comment) - I strongly vote against showing this screen every time a user adds an account. 🙏 |
@tobiasKaminsky I did a bit of code cleanup: c6b2574 😃 Hope you are fine with that |
@jancborchardt all fine for me 👍 |
I'd also vote for adding a different way to sign up for a provider if it's your n-th account (where n > 1), but definitely NOT showing this whole thing. |
public static final String EXTRA_ALLOW_CLOSE = "ALLOW_CLOSE"; | ||
public static final int FIRST_RUN_RESULT_CODE = 199; | ||
|
||
private ProgressIndicator mProgress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather progressIndicator please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed via 0c5b020
|
||
return textView; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed via 0c5b020
Lint
FindBugs (new)
FindBugs (master)
|
@tobiasKaminsky @mario I fixed the latest code review comments. So the only issue remaining is if this should be kept the way it is regarding addition of new accounts when there are already some existing also looking at possible user feedback since this has been backported and shipped with 3.2.2, see #2881 (comment) |
also cc @ardevd for feedback if this is fine behavior-wise |
To get 3.3.0 out the door I am fine with merging this as-is while I'd still hope we can find a better solution post-3.3.0 like showing the login screen directly (after the initial account has been created) and having the provider selection as an option at the bottom of the login screen. What do you think @mario? I am not really happy with the actual state but consider it a trade-off to get 3.3.0 ready for an RC even though there are still some more PRs I'd like to see in 3.3.0 :/ |
also fine as-is hoping for us to find a better solution later |
Signed-off-by: tobiasKaminsky tobias@kaminsky.me