Skip to content

login: Add a basic login flow #39

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

Merged
merged 4 commits into from
Apr 4, 2023
Merged

login: Add a basic login flow #39

merged 4 commits into from
Apr 4, 2023

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Mar 31, 2023

There's plenty more to do to improve the UX of this, and I've
filed several follow-up issues. Most critically, we'll need
to store the resulting credentials on the device (#34), so you don't
have to repeat the login each time you start the app.

But this is enough that it becomes possible to log in at an
account of your choice, making the flow testable end to end.
In particular this makes a useful bootstrapping step toward
the task of storing data locally (#13): with this, there's now some
useful data we could store.

Fixes: #11

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! See a comment below about making the UI update.

Also it would be good to make the new screens behave well with "system intrusions"; do you think we could do that by just putting the whole content of each screen in a SafeArea?

Comment on lines 114 to 122
/// Add an account to the store, returning its assigned account ID.
Future<int> insertAccount(Account account) async {
final accountId = _nextAccountId;
_nextAccountId++;
assert(!_accounts.containsKey(accountId));
_accounts[accountId] = account;
return accountId;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As you pointed out in the office, this should have a notifyListeners call.

@gnprice
Copy link
Member Author

gnprice commented Apr 4, 2023

Thanks!

Also it would be good to make the new screens behave well with "system intrusions"; do you think we could do that by just putting the whole content of each screen in a SafeArea?

Seems likely.

Remind me how you reproduced getting the system intrusions to intrude? ISTR that being in landscape mode; but these lists have max widths that are less than a phone's landscape-mode width, so perhaps that wasn't it.

@chrisbobbe
Copy link
Collaborator

Hmm, I'm not sure I actually reproduced getting the system intrusions to intrude; I just figured it might happen on some screens. You could try removing the max widths and running in landscape mode, yeah.

One thing I think I did notice is that some of the contents weren't horizontally padded even in portrait mode on my iPhone, where there are no system intrusions. To get that padding with and without system intrusions, and without too much padding, I think we've had good results with the SafeArea.minimum property.

@gnprice
Copy link
Member Author

gnprice commented Apr 4, 2023

One thing I think I did notice is that some of the contents weren't horizontally padded even in portrait mode on my iPhone, where there are no system intrusions.

Ah right, I remember that now. Yeah, will do.

@gnprice
Copy link
Member Author

gnprice commented Apr 4, 2023

Revision pushed; PTAL.

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Apr 4, 2023

Thanks, LGTM! There was a hiccup building for manual testing but that's solved: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20version.20solving.20failed/near/1541336

(I'll send that solution in a PR next. Edit: #50)

gnprice added 4 commits April 4, 2023 14:31
This lets the widget class better encapsulate what its route
is meant to look like.  That will help us as we start to do
slightly more complex things with the routes.
For accountEntries, we make use of Dart 3's fancy new "record" types
to keep the API simple to use.
There's plenty more to do to improve the UX of this, and I've
filed several follow-up issues.  Most critically, we'll need
to store the resulting credentials on the device, so you don't
have to repeat the login each time you start the app.

But this is enough that it becomes possible to log in at an
account of your choice, making the flow testable end to end.
In particular this makes a useful bootstrapping step toward
the task of storing data locally: with this, there's now some
useful data we could store.

Fixes: zulip#11
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.

Add a login flow
2 participants