BIT-157: Create account screen #28
Conversation
|
No New Or Fixed Issues Found |
There was a problem hiding this comment.
Awesome!
🎨 Could the screenshot be uploaded again? There seems to be a problem with that, or at least it's not showing on my end.
🎨 Also, could the strings be updated to use the localization file keys given that #27 has now been merged?
| /// The password strength indicator and label indicating the importance of remembering the master password. | ||
| private var passwordStrengthIndicator: some View { | ||
| VStack { | ||
| HStack { | ||
| Text("Important: Your master password cannot be recovered if you forget it! 12 characters minimum.") | ||
| .foregroundColor(Color(asset: Asset.Colors.textSecondary)) | ||
| .font(.system(.footnote)) | ||
|
|
||
| Spacer() | ||
| } | ||
|
|
||
| RoundedRectangle(cornerRadius: 2) | ||
| .frame(height: 8) | ||
| .foregroundColor(Color(asset: Asset.Colors.separatorOpaque)) | ||
| } | ||
| .padding(.bottom, 16) | ||
| } |
There was a problem hiding this comment.
🎨 Could this be extracted to its own file so it can be a reusable control? Additionally, it could internally update the color based on the strength level (I guess this color changing will happen in a different PR).
There was a problem hiding this comment.
Good call 👍 And correct, the color changing will be handled in a different PR.
| func makeBody(configuration: Configuration) -> some View { | ||
| HStack { | ||
| VStack { | ||
| Toggle(configuration) | ||
| .labelsHidden() | ||
| .tint(Color(asset: Asset.Colors.primaryBitwarden)) | ||
| } | ||
| .padding(.trailing, 4) | ||
|
|
||
| description() | ||
|
|
||
| Spacer() | ||
| } | ||
| } |
There was a problem hiding this comment.
🤔 Is the VStack necessary? Given that it has only one child.
There was a problem hiding this comment.
Good catch! Removing the VStack.
| private var passwordStrengthIndicator: some View { | ||
| VStack { | ||
| HStack { | ||
| Text("Important: Your master password cannot be recovered if you forget it! 12 characters minimum.") |
There was a problem hiding this comment.
🤔 Could we move the 12 as a constant of default minimum characters for password declared somewhere else? It will be reused in other places in the app.
Yes, absolutely! Please let me know if you're still having issues seeing the newly uploaded screenshot. |
- Creates PasswordStrengthIndicator view - Updates strings to used localized string - Creates Constants enum for constant values
| @@ -138,7 +120,7 @@ struct CreateAccountView: View { | |||
| DescriptiveToggleStyle( | |||
| description: { | |||
| VStack(alignment: .leading) { | |||
| Text("By activating this switch you agree to the following:") | |||
| Text(Localizations.acceptPolicies) | |||
| .foregroundColor(Color(asset: Asset.Colors.textSecondary)) | |||
| .font(.system(.footnote)) | |||
|
|
|||
🎟️ Tracking
BIT-157
🚧 Type of change
📔 Objective
This PR implements a draft of the create account page. The UI is not final, and will be further stylized in later PRs.
Also worth noting - functionality for the interactive buttons (toggles & show/hide password) will be handled in later PRs.
📋 Code changes
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes