Server selection and login are on different pages. #33

Manually merged
Henry-Hiles merged 8 commits from istalri/nexus:better-login into better-login 2026-06-05 16:44:35 -04:00
Contributor

Solves #27

The login is no longer hidden at the bottom of server selection but on on it's own page and has a similar look and feel as the Verify-Page. Taking the Verify-Page as inspiration was a recommendation from @Henry-Hiles.

Also for the server selection, now only the server cards are scrollable and not the entire page anymore what feels and looks a bit better. For demo and test purposes I left an extra card "Lorem ipsum" so it's easy to demonstrate the new behaviour. This card will obviously vanish in the finished version.

I am not 100% happy with this yet for a couple of reasons.

1. There are no back buttons so once you clicked you have to restart the app if you selected the wrong server by accident.
2. There is no information about what server you are logging in and verifying once you left the server selection.
2. Abusing the ClientStateController while thematically correct seems a bit hacky. Maybe we should have a LoginController, especially with future features for the login in mind.

EDIT: In my opinion we have a good feel now.

Anyway tell me what you think I am open for feedback :) Also attached are some demo videos of the new behaviour and proof that everything works.

Solves #27 The login is no longer hidden at the bottom of server selection but on on it's own page and has a similar look and feel as the Verify-Page. Taking the Verify-Page as inspiration was a recommendation from @Henry-Hiles. Also for the server selection, now only the server cards are scrollable and not the entire page anymore what feels and looks a bit better. For demo and test purposes I left an extra card "Lorem ipsum" so it's easy to demonstrate the new behaviour. This card will obviously vanish in the finished version. ~~I am not 100% happy with this yet for a couple of reasons.~~ ~~1. There are no back buttons so once you clicked you have to restart the app if you selected the wrong server by accident.~~ ~~2. There is no information about what server you are logging in and verifying once you left the server selection.~~ ~~2. Abusing the ClientStateController while thematically correct seems a bit hacky. Maybe we should have a LoginController, especially with future features for the login in mind.~~ EDIT: In my opinion we have a good feel now. Anyway tell me what you think I am open for feedback :) Also attached are some demo videos of the new behaviour and proof that everything works.
istalri force-pushed better-login from 11c85f4a07 to 5ad85d3f62 2026-06-02 04:04:24 -04:00 Compare
Henry-Hiles requested changes 2026-06-02 10:02:43 -04:00
Dismissed
Henry-Hiles left a comment

This mostly looks good, just a couple things I think should be done a different way.

This mostly looks good, just a couple things I think should be done a different way.
@ -9,6 +9,14 @@ class ClientStateController extends Notifier<ClientState?> {
state = newState;
}
void setHomeServer(String homeserver){
Owner

Given this won't be persistent anyways, I think we can go for a simpler approach and not touch client state controller.

Given this won't be persistent anyways, I think we can go for a simpler approach and not touch client state controller.
Henry-Hiles marked this conversation as resolved
lib/main.dart Outdated
@ -125,2 +126,3 @@
if (!clientState.isLoggedIn) {
if ((!clientState.isLoggedIn) && (clientState.homeserverUrl == null || clientState.homeserverUrl?.isEmpty == true)) {
return SelectServerPage();
Owner

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.
Henry-Hiles marked this conversation as resolved
@ -11,4 +8,3 @@
import "package:nexus/widgets/loading.dart";
class LoginPage extends HookConsumerWidget {
const LoginPage({super.key});
Owner

Lets take in final Uri homeserver as an argument here, instead of using ClientStateController.

Lets take in `final Uri homeserver` as an argument here, instead of using `ClientStateController`.
Henry-Hiles marked this conversation as resolved
@ -46,3 +18,1 @@
);
}
isLoading.value = false;
if(homeserverUrl == null || homeserverUrl.trim().isEmpty) {
Owner

This will be redundant when it takes in a URI.

This will be redundant when it takes in a URI.
Henry-Hiles marked this conversation as resolved
@ -197,3 +76,3 @@
);
isLoading.value = false;
isLoggingIn.value = false;
}
Owner

Else: pop page

Else: pop page
Henry-Hiles marked this conversation as resolved
@ -0,0 +48,4 @@
);
} else {
homeserverUrl.text = newHomeserver!.origin;
ref.watch(ClientStateController.provider.notifier).setHomeServer(newUrl);
Owner

Instead of setting homeserver in client state controller, can we just Navigator.of(context).push the LoginPage, passing in a homeserver as an argument?

Instead of setting homeserver in client state controller, can we just `Navigator.of(context).push` the `LoginPage`, passing in a `homeserver` as an argument?
Henry-Hiles marked this conversation as resolved
@ -0,0 +174,4 @@
onPressed: () => launch(Uri.https("servers.joinmatrix.org")),
child: Text("See more homeservers..."),
),
if (isLoading.value)
Owner

This padding should be removed entirely now theres nothing below it. When loading, the whole page should be replaced by a loading indicator, not just at the bottom.

This padding should be removed entirely now theres nothing below it. When loading, the whole page should be replaced by a loading indicator, not just at the bottom.
Henry-Hiles marked this conversation as resolved
pubspec.lock Outdated
@ -836,10 +836,10 @@ packages:
dependency: transitive
Owner

I'd appreciate if you didn't commit these changes.

I'd appreciate if you didn't commit these changes.
@ -0,0 +49,4 @@
);
} else {
homeserverUrl.text = newHomeserver!.origin;
Navigator.push(context, MaterialPageRoute(builder: (_) => LoginPage(homeserver: Uri.parse(newUrl))));
Owner

Might be nice to do that Uri.parse in client.discoverHomeserver, and have that return a Uri.

Might be nice to do that `Uri.parse` in `client.discoverHomeserver`, and have that return a Uri.
Author
Contributor

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.
Owner

Also, you should use Navigator.of(context).push, and await it.

Also, you should use `Navigator.of(context).push`, and await it.
Author
Contributor

Makes sense, will do

Makes sense, will do
Henry-Hiles marked this conversation as resolved
Author
Contributor

Updated UI/UX to look and feel nicer.

Updated UI/UX to look and feel nicer.
lib/main.dart Outdated
@ -125,3 +125,2 @@
if (!clientState.isLoggedIn) {
return LoginPage();
if (!clientState.isLoggedIn) {
Owner

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
Author
Contributor

yeah that one was probably an typo

yeah that one was probably an typo
Henry-Hiles marked this conversation as resolved
@ -20,3 +20,2 @@
final isLoading = useState(false);
final homeserver = useState<String?>(null);
//This is the safe way to request things directly after page load.
Owner

Uh, no it's not. The "safe"/normal way is to set autofocus on the text field.

Uh, no it's not. The "safe"/normal way is to set `autofocus` on the text field.
Author
Contributor

Damn, alright learned something new. Stackoverlfow entries are probably too old :D

Damn, alright learned something new. Stackoverlfow entries are probably too old :D
Henry-Hiles marked this conversation as resolved
@ -47,2 +62,3 @@
isLoggingIn.value = false;
}
isLoading.value = false;
else{
Owner

bad formatting here. make sure you format your code :)

bad formatting here. make sure you format your code :)
Author
Contributor

Yeah I totally forgot. Autoformatting is good? like ctrl + shift + i?

Yeah I totally forgot. Autoformatting is good? like ctrl + shift + i?
Owner

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.
Henry-Hiles marked this conversation as resolved
@ -206,0 +132,4 @@
width: hasError.value ? 4 : 2,
color: hasError.value
? theme.colorScheme.error
: theme.colorScheme.primary
Owner

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?

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?
Author
Contributor

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.
@ -20,0 +16,4 @@
final isLoggingIn = useState(false);
final hasError = useState(false);
final userNameFocusNode = useFocusNode();
final passwordFocusNode = useFocusNode();
Owner

These focusnodes shouldnt be needed. Perhaps we just need a Form()

These focusnodes shouldnt be needed. Perhaps we just need a `Form()`
Author
Contributor

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?
Autoformat for better readable code,
using autofocus instead of useState and focus element
Henry-Hiles changed title from WIP: Server selection and login are on different pages. to Server selection and login are on different pages. 2026-06-02 16:43:04 -04:00
Henry-Hiles requested changes 2026-06-02 16:48:12 -04:00
Dismissed
Henry-Hiles left a comment

Seems fine other than these, I can fix the rest myself. I'd also appreciate if you could fix the merge conflicts.

Seems fine other than these, I can fix the rest myself. I'd also appreciate if you could fix the merge conflicts.
@ -206,0 +60,4 @@
appBar: Appbar(
leading: IconButton(
icon: Icon(Icons.arrow_back),
onPressed: () => Navigator.pop(context),
Owner
- onPressed: () => Navigator.pop(context),
+ onPressed: Navigator.of(context).pop,
```diff - onPressed: () => Navigator.pop(context), + onPressed: Navigator.of(context).pop, ```
istalri marked this conversation as resolved
@ -46,1 +50,4 @@
);
isLoggingIn.value = false;
} else {
Navigator.pop(context);
Owner

prefer Navigator.of(context).pop()

prefer `Navigator.of(context).pop()`
istalri marked this conversation as resolved
Henry-Hiles changed target branch from main to better-login 2026-06-03 10:51:32 -04:00
- Borders only get overwritten in error case
- Navigator.pop(context) -> Navigator.of(context).pop()
- Confirm homeserver und Sign in button disabled as long as error is active
Author
Contributor

I think this it. Tell me what you think and if we are ready to merge or if you don't feel like it because something is not right.

I think this it. Tell me what you think and if we are ready to merge or if you don't feel like it because something is not right.
Henry-Hiles requested changes 2026-06-05 14:32:37 -04:00
Dismissed
Henry-Hiles left a comment

Just this, the rest I'll do myself. Thanks :)

Just this, the rest I'll do myself. Thanks :)
@ -202,0 +60,4 @@
appBar: Appbar(
leading: IconButton(
icon: Icon(Icons.arrow_back),
onPressed: () => Navigator.of(context).pop(),
Owner
- onPressed: () => Navigator.of(context).pop(),
+ onPressed: Navigator.of(context).pop,
```diff - onPressed: () => Navigator.of(context).pop(), + onPressed: Navigator.of(context).pop, ```
istalri marked this conversation as resolved
Henry-Hiles manually merged commit 39a280833f into better-login 2026-06-05 16:44:35 -04:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Nexus/nexus!33
No description provided.