Server selection and login are on different pages. #33
UI/UX improvement for the login flow. You can use enter in all text fields and you get better visual cues like red borders on error.
|
|
@ -9,14 +9,6 @@ class ClientStateController extends Notifier<ClientState?> {
|
|||
state = newState;
|
||||
}
|
||||
|
||||
void setHomeServer(String homeserver){
|
||||
state = .new(isInitialized: state?.isInitialized ?? false,
|
||||
isLoggedIn: state?.isLoggedIn ?? false,
|
||||
isVerified: state?.isVerified ?? false,
|
||||
userId: state?.userId,
|
||||
homeserverUrl: homeserver);
|
||||
}
|
||||
|
||||
static final provider = NotifierProvider<ClientStateController, ClientState?>(
|
||||
|
Henry-Hiles marked this conversation as resolved
Outdated
|
||||
ClientStateController.new,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -11,7 +11,6 @@ import "package:nexus/controllers/shared_prefs_controller.dart";
|
|||
import "package:nexus/helpers/extensions/better_when.dart";
|
||||
import "package:nexus/helpers/extensions/scheme_to_theme.dart";
|
||||
import "package:nexus/pages/chat_page.dart";
|
||||
import "package:nexus/pages/login_page.dart";
|
||||
import "package:nexus/pages/select_server_page.dart";
|
||||
import "package:nexus/pages/verify_page.dart";
|
||||
import "package:nexus/widgets/error_dialog.dart";
|
||||
|
|
@ -124,10 +123,8 @@ class App extends StatelessWidget {
|
|||
return Loading();
|
||||
}
|
||||
|
||||
if ((!clientState.isLoggedIn) && (clientState.homeserverUrl == null || clientState.homeserverUrl?.isEmpty == true)) {
|
||||
if (!clientState.isLoggedIn) {
|
||||
|
Henry-Hiles marked this conversation as resolved
Outdated
Henry-Hiles
commented
you changed this to have two spaces between the ) and {, not sure why you changed this to have two spaces between the ) and {, not sure why
istalri
commented
yeah that one was probably an typo yeah that one was probably an typo
|
||||
return SelectServerPage();
|
||||
} else if(!clientState.isLoggedIn) {
|
||||
return LoginPage();
|
||||
} else if (!clientState.isVerified) {
|
||||
|
Henry-Hiles marked this conversation as resolved
Outdated
Henry-Hiles
commented
I don't think this logic needs to be changed this much. If you can keep the previous logic, but instead show ServerSelectPage instead of login, we can take a different approach to push the login. I don't think this logic needs to be changed this much. If you can keep the previous logic, but instead show ServerSelectPage instead of login, we can take a different approach to push the login.
|
||||
return VerifyPage();
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -2,67 +2,52 @@ import "package:flutter/material.dart";
|
|||
import "package:flutter_hooks/flutter_hooks.dart";
|
||||
import "package:hooks_riverpod/hooks_riverpod.dart";
|
||||
import "package:nexus/controllers/client_controller.dart";
|
||||
import "package:nexus/controllers/client_state_controller.dart";
|
||||
import "package:nexus/models/requests/login_request.dart";
|
||||
import "package:nexus/widgets/appbar.dart";
|
||||
|
Henry-Hiles marked this conversation as resolved
Henry-Hiles
commented
Lets take in Lets take in `final Uri homeserver` as an argument here, instead of using `ClientStateController`.
|
||||
|
||||
class LoginPage extends HookConsumerWidget {
|
||||
const LoginPage({super.key});
|
||||
final Uri homeserver;
|
||||
|
||||
const LoginPage({super.key, required this.homeserver});
|
||||
|
||||
@override
|
||||
Widget build(BuildContext context, WidgetRef ref) {
|
||||
final client = ref.watch(ClientController.provider.notifier);
|
||||
final isLoggingIn = useState(false);
|
||||
final homeserverUrl = ref.watch(ClientStateController.provider)?.homeserverUrl;
|
||||
final hasError = useState(false);
|
||||
final userNameFocusNode = useFocusNode();
|
||||
|
Henry-Hiles marked this conversation as resolved
Outdated
Henry-Hiles
commented
This will be redundant when it takes in a URI. This will be redundant when it takes in a URI.
|
||||
final passwordFocusNode = useFocusNode();
|
||||
|
Henry-Hiles
commented
These focusnodes shouldnt be needed. Perhaps we just need a These focusnodes shouldnt be needed. Perhaps we just need a `Form()`
istalri
commented
TODO: This is still open but you said you wanted to look into it. Is this still the case? TODO: This is still open but you said you wanted to look into it. Is this still the case?
|
||||
|
||||
if(homeserverUrl == null || homeserverUrl.trim().isEmpty) {
|
||||
throw Exception("Homeserver URL must be set for login.");
|
||||
}
|
||||
//This is the safe way to request things directly after page load.
|
||||
|
Henry-Hiles marked this conversation as resolved
Outdated
Henry-Hiles
commented
Uh, no it's not. The "safe"/normal way is to set Uh, no it's not. The "safe"/normal way is to set `autofocus` on the text field.
istalri
commented
Damn, alright learned something new. Stackoverlfow entries are probably too old :D Damn, alright learned something new. Stackoverlfow entries are probably too old :D
|
||||
useEffect(() {
|
||||
WidgetsBinding.instance.addPostFrameCallback((_) {
|
||||
userNameFocusNode.requestFocus();
|
||||
});
|
||||
return null;
|
||||
}, []);
|
||||
|
||||
final theme = Theme.of(context);
|
||||
|
||||
final username = useTextEditingController();
|
||||
final password = useTextEditingController();
|
||||
|
||||
return Scaffold(
|
||||
appBar: Appbar(),
|
||||
body: AlertDialog(
|
||||
title: Text("Login"),
|
||||
content: Column(
|
||||
mainAxisSize: MainAxisSize.min,
|
||||
crossAxisAlignment: CrossAxisAlignment.start,
|
||||
children: [
|
||||
Text(
|
||||
"Enter your login credentials:",
|
||||
),
|
||||
SizedBox(height: 12),
|
||||
TextField(
|
||||
decoration: InputDecoration(label: Text("Username")),
|
||||
controller: username,
|
||||
),
|
||||
SizedBox(height: 12),
|
||||
TextField(
|
||||
decoration: InputDecoration(label: Text("Password")),
|
||||
controller: password,
|
||||
obscureText: true,
|
||||
),
|
||||
],
|
||||
),
|
||||
actions: [
|
||||
TextButton(
|
||||
onPressed: isLoggingIn.value
|
||||
? null
|
||||
: () async {
|
||||
Future<void> tryLogin() async {
|
||||
if (isLoggingIn.value) return;
|
||||
isLoggingIn.value = true;
|
||||
hasError.value = false;
|
||||
|
||||
final error = await client.login(
|
||||
LoginRequest(
|
||||
username: username.text,
|
||||
password: password.text,
|
||||
homeserverUrl: homeserverUrl,
|
||||
),
|
||||
homeserverUrl: homeserver.origin
|
||||
)
|
||||
);
|
||||
|
||||
if (error != null && context.mounted) {
|
||||
if (!context.mounted) return;
|
||||
|
||||
if (error != null) {
|
||||
hasError.value = true;
|
||||
ScaffoldMessenger.of(context).showSnackBar(
|
||||
SnackBar(
|
||||
content: Text(
|
||||
|
|
@ -76,7 +61,89 @@ class LoginPage extends HookConsumerWidget {
|
|||
);
|
||||
isLoggingIn.value = false;
|
||||
}
|
||||
|
istalri marked this conversation as resolved
Outdated
Henry-Hiles
commented
```diff
- onPressed: () => Navigator.pop(context),
+ onPressed: Navigator.of(context).pop,
```
istalri marked this conversation as resolved
Outdated
Henry-Hiles
commented
```diff
- onPressed: () => Navigator.of(context).pop(),
+ onPressed: Navigator.of(context).pop,
```
|
||||
else{
|
||||
|
Henry-Hiles marked this conversation as resolved
Outdated
Henry-Hiles
commented
bad formatting here. make sure you format your code :) bad formatting here. make sure you format your code :)
istalri
commented
Yeah I totally forgot. Autoformatting is good? like ctrl + shift + i? Yeah I totally forgot. Autoformatting is good? like ctrl + shift + i?
Henry-Hiles
commented
Yeah, that should work great. I have it set to format on save. Yeah, that should work great. I have it set to format on save.
|
||||
Navigator.pop(context);
|
||||
|
istalri marked this conversation as resolved
Henry-Hiles
commented
prefer prefer `Navigator.of(context).pop()`
|
||||
}
|
||||
passwordFocusNode.requestFocus();
|
||||
isLoggingIn.value = false;
|
||||
}
|
||||
|
||||
return Scaffold(
|
||||
appBar: Appbar(
|
||||
leading: IconButton(
|
||||
icon: Icon(Icons.arrow_back),
|
||||
onPressed: () => Navigator.pop(context)
|
||||
),
|
||||
),
|
||||
body: AlertDialog(
|
||||
title: Text("Login to ${homeserver.host}"),
|
||||
content: Column(
|
||||
mainAxisSize: MainAxisSize.min,
|
||||
crossAxisAlignment: CrossAxisAlignment.start,
|
||||
children: [
|
||||
Text(
|
||||
"Enter your login credentials:",
|
||||
),
|
||||
SizedBox(height: 12),
|
||||
TextField(
|
||||
focusNode: userNameFocusNode,
|
||||
textInputAction: TextInputAction.next,
|
||||
onChanged: (newVal) {
|
||||
if (hasError.value) {
|
||||
hasError.value = false;
|
||||
}
|
||||
},
|
||||
decoration: InputDecoration(
|
||||
label: Text("Username"),
|
||||
enabledBorder: OutlineInputBorder(
|
||||
borderSide: BorderSide(
|
||||
width: hasError.value ? 4 : 2,
|
||||
color: hasError.value
|
||||
? theme.colorScheme.error
|
||||
: theme.colorScheme.primary
|
||||
),
|
||||
)
|
||||
),
|
||||
controller: username,
|
||||
),
|
||||
SizedBox(height: 12),
|
||||
TextField(
|
||||
focusNode: passwordFocusNode,
|
||||
textInputAction: TextInputAction.done,
|
||||
onSubmitted: (_) => tryLogin(),
|
||||
onChanged: (newVal) {
|
||||
if (hasError.value) {
|
||||
hasError.value = false;
|
||||
}
|
||||
},
|
||||
selectAllOnFocus: true,
|
||||
decoration: InputDecoration(
|
||||
label: Text("Password"),
|
||||
focusedBorder: OutlineInputBorder(
|
||||
borderSide: BorderSide(
|
||||
width: hasError.value ? 4 : 2,
|
||||
color: hasError.value
|
||||
? theme.colorScheme.error
|
||||
: theme.colorScheme.primary
|
||||
)
|
||||
),
|
||||
enabledBorder: OutlineInputBorder(
|
||||
borderSide: BorderSide(
|
||||
width: hasError.value ? 4 : 2,
|
||||
color: hasError.value
|
||||
? theme.colorScheme.error
|
||||
: theme.colorScheme.primary
|
||||
|
Henry-Hiles
commented
Please keep width as default always, and only override the border at all if Please keep width as default always, and only override the border at all if `hasError` is true. Also, does both `focusedBorder` and `enabledBorder` need to be set?
istalri
commented
I guess it depends on how you see it. Technically for the userfield you only need enabledBorder and for the passwordfield you only need focusedBorder. But when you then click into the userfield both error borders will disappear until you click back into the passwordfield. I guess it depends on how you see it. Technically for the userfield you only need enabledBorder and for the passwordfield you only need focusedBorder. But when you then click into the userfield both error borders will disappear until you click back into the passwordfield.
|
||||
),
|
||||
)
|
||||
),
|
||||
controller: password,
|
||||
obscureText: true,
|
||||
),
|
||||
],
|
||||
),
|
||||
actions: [
|
||||
TextButton(
|
||||
onPressed: () => tryLogin(),
|
||||
child: Text("Sign In"),
|
||||
),
|
||||
],
|
||||
|
|
|
|||
|
|
@ -3,9 +3,9 @@ import "package:flutter_hooks/flutter_hooks.dart";
|
|||
import "package:flutter_svg/flutter_svg.dart";
|
||||
import "package:hooks_riverpod/hooks_riverpod.dart";
|
||||
import "package:nexus/controllers/client_controller.dart";
|
||||
import "package:nexus/controllers/client_state_controller.dart";
|
||||
import "package:nexus/helpers/launch_helper.dart";
|
||||
import "package:nexus/models/homeserver.dart";
|
||||
import "package:nexus/pages/login_page.dart";
|
||||
import "package:nexus/widgets/appbar.dart";
|
||||
import "package:nexus/widgets/divider_text.dart";
|
||||
import "package:nexus/widgets/loading.dart";
|
||||
|
|
@ -17,11 +17,11 @@ class SelectServerPage extends HookConsumerWidget {
|
|||
Widget build(BuildContext context, WidgetRef ref) {
|
||||
final theme = Theme.of(context);
|
||||
final client = ref.watch(ClientController.provider.notifier);
|
||||
|
||||
final hasError = useState(false);
|
||||
final isLoading = useState(false);
|
||||
final homeserverFocusNode = useFocusNode();
|
||||
|
||||
final launch = ref.watch(LaunchHelper.provider).launchUrl;
|
||||
|
||||
final homeserverUrl = useTextEditingController();
|
||||
|
||||
Future<void> setHomeserver(Uri? newHomeserver) async {
|
||||
|
|
@ -37,6 +37,7 @@ class SelectServerPage extends HookConsumerWidget {
|
|||
|
||||
if (context.mounted) {
|
||||
if (newUrl == null) {
|
||||
hasError.value = true;
|
||||
ScaffoldMessenger.of(context).showSnackBar(
|
||||
SnackBar(
|
||||
content: Text(
|
||||
|
|
@ -48,15 +49,19 @@ class SelectServerPage extends HookConsumerWidget {
|
|||
);
|
||||
} else {
|
||||
homeserverUrl.text = newHomeserver!.origin;
|
||||
|
Henry-Hiles marked this conversation as resolved
Henry-Hiles
commented
Instead of setting homeserver in client state controller, can we just Instead of setting homeserver in client state controller, can we just `Navigator.of(context).push` the `LoginPage`, passing in a `homeserver` as an argument?
|
||||
ref.watch(ClientStateController.provider.notifier).setHomeServer(newUrl);
|
||||
Navigator.push(context, MaterialPageRoute(builder: (_) => LoginPage(homeserver: Uri.parse(newUrl))));
|
||||
|
Henry-Hiles marked this conversation as resolved
Outdated
Henry-Hiles
commented
Might be nice to do that Might be nice to do that `Uri.parse` in `client.discoverHomeserver`, and have that return a Uri.
istalri
commented
Yeah I don't mind. If you feel that's cleaner I will do so. Yeah I don't mind. If you feel that's cleaner I will do so.
Henry-Hiles
commented
Also, you should use Also, you should use `Navigator.of(context).push`, and await it.
istalri
commented
Makes sense, will do Makes sense, will do
|
||||
}
|
||||
}
|
||||
|
||||
homeserverFocusNode.requestFocus();
|
||||
isLoading.value = false;
|
||||
}
|
||||
|
||||
return Scaffold(
|
||||
appBar: Appbar(),
|
||||
body: Center(
|
||||
body: isLoading.value
|
||||
? const Loading()
|
||||
: Center(
|
||||
child: ConstrainedBox(
|
||||
constraints: BoxConstraints(maxWidth: 600),
|
||||
child: Column(
|
||||
|
|
@ -90,14 +95,33 @@ class SelectServerPage extends HookConsumerWidget {
|
|||
children: [
|
||||
Expanded(
|
||||
child: TextField(
|
||||
focusNode: homeserverFocusNode,
|
||||
textInputAction: TextInputAction.done,
|
||||
onSubmitted: (_) => setHomeserver(Uri.tryParse(homeserverUrl.text)),
|
||||
onChanged: (newVal) {
|
||||
if (hasError.value) {
|
||||
hasError.value = false;
|
||||
}
|
||||
},
|
||||
controller: homeserverUrl,
|
||||
decoration: InputDecoration(
|
||||
labelText: "Homeserver URL (e.g. matrix.org)",
|
||||
hintText: "e.g. matrix.org",
|
||||
enabledBorder: OutlineInputBorder(
|
||||
borderSide: BorderSide(color: (homeserverUrl.text.trim().isEmpty)
|
||||
focusedBorder: OutlineInputBorder(
|
||||
borderSide: BorderSide(
|
||||
width: hasError.value ? 4 : 2,
|
||||
color: hasError.value
|
||||
? theme.colorScheme.error
|
||||
: theme.colorScheme.primary)
|
||||
: theme.colorScheme.primary
|
||||
)
|
||||
),
|
||||
enabledBorder: OutlineInputBorder(
|
||||
borderSide: BorderSide(
|
||||
width: hasError.value ? 4 : 2,
|
||||
color: hasError.value
|
||||
? theme.colorScheme.error
|
||||
: theme.colorScheme.primary
|
||||
)
|
||||
),
|
||||
),
|
||||
),
|
||||
|
|
@ -174,10 +198,6 @@ class SelectServerPage extends HookConsumerWidget {
|
|||
onPressed: () => launch(Uri.https("servers.joinmatrix.org")),
|
||||
child: Text("See more homeservers..."),
|
||||
),
|
||||
if (isLoading.value)
|
||||
Padding(padding: EdgeInsets.only(top: 32), child: Loading())
|
||||
else
|
||||
Padding(padding: EdgeInsets.only(top: 12))
|
||||
],
|
||||
),
|
||||
),
|
||||
|
|
|
|||
Given this won't be persistent anyways, I think we can go for a simpler approach and not touch client state controller.