Add email verification screen #6125
Conversation
Make a reusable onboarding icon view.
Refine use of MainActor.
|
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/jQNUE3 |
pixlwave
requested review from
a team,
ismailgulek,
aringenbach and
Anderas
and removed request for
a team and
ismailgulek
RiotSwiftUI/Modules/Authentication/VerifyEmail/AuthenticationVerifyEmailViewModel.swift
Outdated
Show resolved
Hide resolved
| override func process(viewAction: AuthenticationVerifyEmailViewAction) { | ||
| switch viewAction { | ||
| case .send: | ||
| Task { await completion?(.send(state.bindings.emailAddress)) } |
There was a problem hiding this comment.
It seems that Action will often be a close 1:1 to Result, I wonder if we could avoid this by declaring distinct actions on the view model instead, cutting out one layer of indirection. So instead of callback, we would have sendAction, resendAction and cancelAction, something along the lines of this. What do you think?
There was a problem hiding this comment.
Whilst I very much like the idea of this, I'm not sure it's worth changing for this screen as it will
- break consistency with all other SwiftUI screens which will could make maintaining it harder.
- remove an easy place to insert a view action that doesn't need to interact with coordinator if one is needed in the future.
There was a problem hiding this comment.
We would still retain Action (some of which don't need to interact with coordinators), only cut out the Result type which only calls some internal function on the coordinator. So in essence the body of that internal method would be assigned directly as actionCallback. But as you say, it is fine to keep for consistency sake and see how it evolved with scale
| /// - Parameters: | ||
| /// - label: The label to show on the indicator. | ||
| /// - isInteractionBlocking: Whether the indicator should block any user interaction. | ||
| @MainActor private func startLoading(label: String = VectorL10n.loading, isInteractionBlocking: Bool = true) { |
There was a problem hiding this comment.
We don't seem to be using nondefault parameters in this class and the method is private, so maybe they are not needed?
There was a problem hiding this comment.
Updated. Worth saying that this is how the template generates the function, so most screens will probably have this method left in this format.
There was a problem hiding this comment.
Hm then we might have to re-evaluate how we use the templates. In my understanding templates should both speed up the creation of repeated classes as well as help attain some amount of consistency, but at the end of the day it is the final code that matters, and how self-evident it is when we read it a year from now (when the template origin is long obscured).
RiotSwiftUI/Modules/Authentication/VerifyEmail/View/AuthenticationVerifyEmailScreen.swift
Outdated
Show resolved
Hide resolved
...I/Modules/Authentication/VerifyEmail/Test/Unit/AuthenticationVerifyEmailViewModelTests.swift
Outdated
Show resolved
Hide resolved
RiotSwiftUI/Modules/Authentication/VerifyEmail/View/AuthenticationVerifyEmailScreen.swift
Outdated
Show resolved
Hide resolved
|
Updated with a |
This PR adds the views, but it isn't yet part of the flow and the coordinator still requires an implementation of polling for the homeserver to confirm when the link was tapped.
Issue: #5649