Skip to content
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

feat: Sign in Window views & view models #20

Merged
merged 2 commits into from
Feb 7, 2025
Merged

Conversation

spikecurtis
Copy link
Collaborator

@spikecurtis spikecurtis commented Feb 7, 2025

Adds views for the Sign In dialog window, and a backing view model.

This is still a bit rough around the edges, but I'd like to get some eyes on it and merge to iterate.

It also needs to be integrated with some of the structural changes in #19 including dependency injection. That can happen per or post merge, depending on when #19 lands in main.

Known issues:

  • Input validation. If you give an invalid URL, then clicking "Next" will crash the app since the hyperlink button won't accept an invalid URI.
  • Relatively janky Error message handling -- doesn't use any consistent styling.

Screenshot 2025-02-07 121242.png

Screenshot 2025-02-07 121347.png

Copy link
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@spikecurtis spikecurtis marked this pull request as ready for review February 7, 2025 08:14
Orientation="Horizontal"
HorizontalAlignment="Center"
Spacing="10">
<Button Content="Back" HorizontalAlignment="Right"
Copy link
Member

Choose a reason for hiding this comment

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

I think the better way of doing forwards/backwards navigation is to use a <RootFrame> in the window rather than setting Window.Content directly.

One pitfall that might make it hard to use the RootFrame's built-in navigation stuff is that I don't think it lets you instantiate the Window types yourself - which makes it hard to get a ViewModel in there.

In the TrayWindow code in my PR, I ended up just setting RootFrame.Content directly because of this limitation (and because I don't need forwards/backwards navigation). But if you could get that working it'd be more idiomatic I think

Copy link
Member

Choose a reason for hiding this comment

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

I switched it to using a Frame so we're not just touching the window content directly, but I'm not using the forward/backwards functionality yet because of the aforementioned issue.

In the future if we solve it, we could have the "Coder Desktop" text and the navigation buttons be on the Window itself, rather than in each page.

@deansheather deansheather merged commit 641f1bc into main Feb 7, 2025
4 checks passed
@deansheather deansheather deleted the spike/sign-in-views branch February 7, 2025 15:10
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.

2 participants