-
Notifications
You must be signed in to change notification settings - Fork 497
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
Add the FTUE use case screen for new users. #5467
Conversation
715ab3a
to
01f52ad
Compare
LGTM. The only feedback I have is that at some point of time we changed the options to: Friends and family Besides that, all looks fine to me. TY! |
A couple of points to note so far @amshakal:
|
|
743f574
to
26258c3
Compare
Update strings. Show the custom server field as needed. Enable scroll edges appearance for white navigation bar.
26258c3
to
906eb48
Compare
Updated strings and navigation bar colour is visible in this recording along with what the navigation looks like from the UseCase1080.mp4 |
📱 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/xYJsPK |
@@ -62,6 +62,10 @@ final class AuthenticationCoordinator: NSObject, AuthenticationCoordinatorProtoc | |||
authenticationViewController.authType = authenticationType | |||
} | |||
|
|||
func showCustomServer() { | |||
authenticationViewController.hideCustomServers(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably rename this to something along the lines of setCustomServersVisibility(_ visible: Bool)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with setCustomServerFieldsVisible(_ isVisible: Bool)
in the end and left it as it on the Coordinator for now - there will be further changes to this when AuthVC is replaced so probably makes sense to hold back and see what makes sense at that stage.
/// - tappableText: The tappable text that will be substituted into the `%@` placeholder. | ||
/// - action: The action to perform when tapping the button. | ||
internal init(_ mainText: String, tappableText: String, action: @escaping () -> Void) { | ||
guard let range = mainText.range(of: "%@") else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if a better interface here would be to receive both the full string and the tappable string and search for the range of one in the other, instead of relying on a %@
format placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes far more sense now that you mention it 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although... In theory this could produce the wrong result if the tappable text is repeated and the first occurrence isn't meant to be the one to tap on. 🤔
Terrible contrived example:
For more information tap the link below, otherwise contact us via this link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, no, that makes sense. Perhaps then the full text and the tappable range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just pushes the issue further up but it still exists. If you have VectorL10n.descriptionMainText
and VectorL10n.descriptionTappableText
, you would need to either
- Get the main string with the placeholder as
%@
. - Create a range from the start of the placeholder and the length of the tappable string.
- Manually substitute the placeholder with the tappable string
- Pass those to
InlineTextButton
or
- Get the main string with the tappable string already substituted.
- Somehow handle the case of multiple occurrences of the tappable string (in all languages).
I agree it isn't particularly tidy this way, but not sure of any other options really 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
Fixes #5160. I will add analytics separately in another PR
Additionally adds
InlineTextButton
andStyledText
SwiftUI views and replaces some custom code in the Analytics prompt with this button.