Server selection and login are on different pages. #33
No reviewers
Labels
No labels
Blocked
Good First Issue
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Nexus/nexus!33
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "istalri/nexus:better-login"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
11c85f4a07to5ad85d3f62This 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){Given this won't be persistent anyways, I think we can go for a simpler approach and not touch client state controller.
@ -125,2 +126,3 @@if (!clientState.isLoggedIn) {if ((!clientState.isLoggedIn) && (clientState.homeserverUrl == null || clientState.homeserverUrl?.isEmpty == true)) {return SelectServerPage();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.
@ -11,4 +8,3 @@import "package:nexus/widgets/loading.dart";class LoginPage extends HookConsumerWidget {const LoginPage({super.key});Lets take in
final Uri homeserveras an argument here, instead of usingClientStateController.@ -46,3 +18,1 @@);}isLoading.value = false;if(homeserverUrl == null || homeserverUrl.trim().isEmpty) {This will be redundant when it takes in a URI.
@ -197,3 +76,3 @@);isLoading.value = false;isLoggingIn.value = false;}Else: pop page
@ -0,0 +48,4 @@);} else {homeserverUrl.text = newHomeserver!.origin;ref.watch(ClientStateController.provider.notifier).setHomeServer(newUrl);Instead of setting homeserver in client state controller, can we just
Navigator.of(context).pushtheLoginPage, passing in ahomeserveras an argument?@ -0,0 +174,4 @@onPressed: () => launch(Uri.https("servers.joinmatrix.org")),child: Text("See more homeservers..."),),if (isLoading.value)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.
@ -836,10 +836,10 @@ packages:dependency: transitiveI'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))));Might be nice to do that
Uri.parseinclient.discoverHomeserver, and have that return a Uri.Yeah I don't mind. If you feel that's cleaner I will do so.
Also, you should use
Navigator.of(context).push, and await it.Makes sense, will do
Updated UI/UX to look and feel nicer.
@ -125,3 +125,2 @@if (!clientState.isLoggedIn) {return LoginPage();if (!clientState.isLoggedIn) {you changed this to have two spaces between the ) and {, not sure why
yeah that one was probably an typo
@ -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.Uh, no it's not. The "safe"/normal way is to set
autofocuson the text field.Damn, alright learned something new. Stackoverlfow entries are probably too old :D
@ -47,2 +62,3 @@isLoggingIn.value = false;}isLoading.value = false;else{bad formatting here. make sure you format your code :)
Yeah I totally forgot. Autoformatting is good? like ctrl + shift + i?
Yeah, that should work great. I have it set to format on save.
@ -206,0 +132,4 @@width: hasError.value ? 4 : 2,color: hasError.value? theme.colorScheme.error: theme.colorScheme.primaryPlease keep width as default always, and only override the border at all if
hasErroris true. Also, does bothfocusedBorderandenabledBorderneed to be set?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();These focusnodes shouldnt be needed. Perhaps we just need a
Form()TODO: This is still open but you said you wanted to look into it. Is this still the case?
WIP: Server selection and login are on different pages.to Server selection and login are on different pages.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),@ -46,1 +50,4 @@);isLoggingIn.value = false;} else {Navigator.pop(context);prefer
Navigator.of(context).pop()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.
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(),