UI - onboarding styling and colors #131
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
LDKNodeMonday/Assets.xcassets/AccentColor.colorset/Contents.json (1)
5-11: Consider using consistent hex notation for color components.The color definition looks good, but for better maintainability, consider using hexadecimal notation consistently across both color definitions.
"components" : { "alpha" : "1.000", - "blue" : "0.894", - "green" : "0.188", - "red" : "0.111" + "blue" : "0xE4", + "green" : "0x30", + "red" : "0x1C" }LDKNodeMonday/View/Home/OnboardingView.swift (2)
28-38: Add accessibility label for the wallet icon.While the styling looks good, consider adding an accessibility label to the image for better VoiceOver support.
Image(systemName: "bolt.horizontal.fill") .resizable() .aspectRatio(contentMode: .fit) .foregroundColor(.accent) .frame(width: 150, height: 150, alignment: .center) + .accessibilityLabel("Monday Wallet Logo") .padding(40)
Line range hint
44-83: Consider responsive padding for different screen sizes.The fixed horizontal padding of 50 points might be too large on smaller devices. Consider using a more responsive approach.
-}.padding(.horizontal, 50) +}.padding(.horizontal, min(50, UIScreen.main.bounds.width * 0.15))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- LDKNodeMonday.xcodeproj/project.pbxproj (6 hunks)
- LDKNodeMonday/Assets.xcassets/AccentColor.colorset/Contents.json (2 hunks)
- LDKNodeMonday/View/Home/OnboardingView.swift (2 hunks)
🔇 Additional comments (1)
LDKNodeMonday/View/Home/OnboardingView.swift (1)
119-128: LGTM! Error handling implementation looks good.The alert implementation provides clear error feedback with proper dismissal handling.
| "color-space" : "display-p3", | ||
| "components" : { | ||
| "alpha" : "1.000", | ||
| "blue" : "0xCD", | ||
| "green" : "0xF3", | ||
| "red" : "0x77" | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Dark mode accent color needs adjustment for better accessibility
The current dark mode accent color (77, F3, CD) has a very high brightness value, particularly in the green channel (F3). This combination:
- Has a relative luminance that may not provide sufficient contrast with light text
- Uses excessive brightness (especially green at 0xF3) that could cause visual discomfort in dark mode
- May not meet WCAG 2.1 contrast requirements for text legibility
Recommendations:
- Reduce the green channel value (0xF3) significantly for dark mode
- Consider using a more muted variant while maintaining the same hue
- Test with both light and dark text against this background color
🔗 Analysis chain
Verify dark mode color contrast and brightness.
The selected color (0x77, 0xF3, 0xCD) appears quite bright for dark mode, which might cause eye strain or readability issues. Please verify that:
- The color provides sufficient contrast with text
- The brightness level is comfortable for dark mode viewing
- The color meets WCAG accessibility guidelines
Let's check if this color is used with text in the onboarding view:
Let me gather more information about where and how this accent color is specifically used in the UI.
Let me check if there are any dark mode specific color assets or alternative color sets defined for this accent color.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of accent color with text in OnboardingView
rg -l "\.accent" | xargs rg "Text|Label"
Length of output: 885
Script:
#!/bin/bash
# Search for accent color usage in SwiftUI views
rg -A 3 -B 3 "\.accentColor|AccentColor"
# Also check for any color scheme or dark mode specific handling
rg -A 3 "preferredColorScheme|\.colorScheme"
Length of output: 1615
Script:
#!/bin/bash
# Check if there are any dark mode specific color definitions in the accent color asset
cat "LDKNodeMonday/Assets.xcassets/AccentColor.colorset/Contents.json"
# Look for any other color assets that might be related
fd colorset LDKNodeMonday/Assets.xcassets -x cat {}/Contents.json
Length of output: 1548
| Button("Create wallet") { | ||
| viewModel.saveSeed() | ||
| isFirstTime = false | ||
| } label: { | ||
| Text("Start Node") | ||
| .bold() | ||
| .foregroundColor(Color(uiColor: UIColor.systemBackground)) | ||
| .frame(maxWidth: .infinity) | ||
| .minimumScaleFactor(0.9) | ||
| .padding(.all, 8) | ||
| } | ||
| .frame(width: 200, height: 25) | ||
| .buttonStyle(BitcoinFilled(tintColor: viewModel.buttonColor, isCapsule: true)) | ||
| .disabled(!isFirstTime && viewModel.seedPhrase.isEmpty) | ||
| .padding(.all, 25) | ||
| .buttonStyle( | ||
| BitcoinFilled( | ||
| tintColor: .accent, | ||
| isCapsule: true | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state to wallet creation button.
Consider adding a loading state to provide feedback during wallet creation process.
+@State private var isCreatingWallet = false
+
Button("Create wallet") {
+ isCreatingWallet = true
viewModel.saveSeed()
isFirstTime = false
}
.buttonStyle(
BitcoinFilled(
tintColor: .accent,
isCapsule: true
)
)
+.disabled(isCreatingWallet)
+.overlay {
+ if isCreatingWallet {
+ ProgressView()
+ .tint(.accent)
+ }
+}Committable suggestion was skipped due to low confidence.
| TextField( | ||
| isFirstTime | ||
| ? "24 word seed phrase (optional)" : "24 word seed phrase (required)", | ||
| text: $viewModel.seedPhrase | ||
| ) | ||
| .textFieldStyle(.roundedBorder) | ||
| .submitLabel(.done) | ||
| .padding(.horizontal, 50) | ||
| .padding(.vertical, 10) | ||
| if viewModel.seedPhraseArray != [] { | ||
| SeedPhraseView( | ||
| words: viewModel.seedPhraseArray, | ||
| preferredWordsPerRow: 2, | ||
| usePaging: true, | ||
| wordsPerPage: 6 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security and user experience of seed phrase input.
Consider adding appropriate keyboard type and content type specifications for better security and user experience.
TextField(
isFirstTime
? "24 word seed phrase (optional)" : "24 word seed phrase (required)",
text: $viewModel.seedPhrase
)
.textFieldStyle(.roundedBorder)
.submitLabel(.done)
+.keyboardType(.asciiCapable)
+.textContentType(.none)
+.autocorrectionDisabled(true)
+.autocapitalization(.none)
.padding(.horizontal, 50)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TextField( | |
| isFirstTime | |
| ? "24 word seed phrase (optional)" : "24 word seed phrase (required)", | |
| text: $viewModel.seedPhrase | |
| ) | |
| .textFieldStyle(.roundedBorder) | |
| .submitLabel(.done) | |
| .padding(.horizontal, 50) | |
| .padding(.vertical, 10) | |
| if viewModel.seedPhraseArray != [] { | |
| SeedPhraseView( | |
| words: viewModel.seedPhraseArray, | |
| preferredWordsPerRow: 2, | |
| usePaging: true, | |
| wordsPerPage: 6 | |
| TextField( | |
| isFirstTime | |
| ? "24 word seed phrase (optional)" : "24 word seed phrase (required)", | |
| text: $viewModel.seedPhrase | |
| ) | |
| .textFieldStyle(.roundedBorder) | |
| .submitLabel(.done) | |
| .keyboardType(.asciiCapable) | |
| .textContentType(.none) | |
| .autocorrectionDisabled(true) | |
| .autocapitalization(.none) | |
| .padding(.horizontal, 50) | |
| .padding(.vertical, 10) | |
| if viewModel.seedPhraseArray != [] { | |
| SeedPhraseView( | |
| words: viewModel.seedPhraseArray, | |
| preferredWordsPerRow: 2, | |
| usePaging: true, | |
| wordsPerPage: 6 |
| 65875A912CCB9809000D3E70 /* XCRemoteSwiftPackageReference "ldk-node" */ = { | ||
| isa = XCRemoteSwiftPackageReference; | ||
| repositoryURL = "https://github.com/lightningdevkit/ldk-node.git"; | ||
| requirement = { | ||
| kind = exactVersion; | ||
| version = 0.4.0; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a version range for LDKNode dependency.
While pinning to exact version 0.4.0 provides stability, it may cause you to miss important security updates. Consider using a version range that allows minor updates.
Update the package requirement to allow minor version updates:
requirement = {
- kind = exactVersion;
- version = 0.4.0;
+ kind = upToNextMinor;
+ minimumVersion = "0.4.0";
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 65875A912CCB9809000D3E70 /* XCRemoteSwiftPackageReference "ldk-node" */ = { | |
| isa = XCRemoteSwiftPackageReference; | |
| repositoryURL = "https://github.com/lightningdevkit/ldk-node.git"; | |
| requirement = { | |
| kind = exactVersion; | |
| version = 0.4.0; | |
| }; | |
| }; | |
| 65875A912CCB9809000D3E70 /* XCRemoteSwiftPackageReference "ldk-node" */ = { | |
| isa = XCRemoteSwiftPackageReference; | |
| repositoryURL = "https://github.com/lightningdevkit/ldk-node.git"; | |
| requirement = { | |
| kind = upToNextMinor; | |
| minimumVersion = "0.4.0"; | |
| }; | |
| }; |
| @@ -173,7 +173,7 @@ | |||
| isa = PBXFrameworksBuildPhase; | |||
| buildActionMask = 2147483647; | |||
| files = ( | |||
| AEFA825E2CC15C7C0095B0BA /* LDKNode in Frameworks */, | |||
| 65875A932CCB9809000D3E70 /* LDKNode in Frameworks */, | |||
There was a problem hiding this comment.
Remove duplicate LDKNode framework references.
There are multiple framework references to LDKNode which can cause build issues. Only one reference should be maintained.
Remove the duplicate references and keep only one:
- 65875A932CCB9809000D3E70 /* LDKNode in Frameworks */,
- AE060C382C051B59006724F1 /* LDKNode in Frameworks */,
- AE80C2052C4AB5E4006E7193 /* LDKNode in Frameworks */,
+ 65875A932CCB9809000D3E70 /* LDKNode in Frameworks */,Committable suggestion was skipped due to low confidence.
|
Thanks for the side-by-side screenshots! ColorColor change looks good, but there are some additional considerations I'm thinking about it:
Overall I'm good with the proposed color change though for now, but do think it should be made across the entire app for cohesion. Casinghttps://developer.apple.com/design/human-interface-guidelines/writing https://developer.apple.com/design/human-interface-guidelines/buttons I’m definitely not against apps that use Sentence case (one of my favorite bitcoin apps uses it), and don’t think Apple even thinks one is always better than the other, but it does seem like the strongest case for which would be the standard for an iOS wallet would be Title (per combination of HIG+Wallet)? UIDoes not work well on non-DynamicType or with Dynamic Type (even though I'm not as explicit about that here (yet) as I am in BDKSwiftExampleWallet), here are screenshots of proposed changes: 
TextI'm good with changing "create wallet" from "start node", good change! OtherWould encourage continuing discussion on each of these as separate Issues/PR's because this PR discussion might get too large and span too many different topics (and possibly opening up Issues before PR's for these items) Would love your help on some existing ui changes like #133 or large feature experimentation/implemenation like #132 I appreciate the experimentation on the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
LDKNodeMonday/View/Home/OnboardingView.swift (2)
31-43: LGTM! Consider enhancing accessibility.The styling follows iOS design guidelines. Consider adding accessibility identifiers for UI testing.
Image(systemName: "bolt.horizontal.fill") .resizable() .aspectRatio(contentMode: .fit) .foregroundColor(.accent) .frame(width: 150, height: 150, alignment: .center) .padding(40) + .accessibilityIdentifier("onboarding-logo")
Line range hint
49-89: Remove commented code.Remove the commented picker implementation to maintain clean production code. If needed, this code can be retrieved from version control history.
- // Default picker style - /* - HStack { - Text("Network") - Spacer() - Picker( - "Network", - selection: $viewModel.selectedNetwork - ) { - Text("Signet").tag(Network.signet) - Text("Testnet").tag(Network.testnet) - } - .pickerStyle(.automatic) - .tint(.accent) - .accessibilityLabel("Select bitcoin network") - } - [... rest of commented code ...] - */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
LDKNodeMonday/View/Home/OnboardingView.swift(3 hunks)
🔇 Additional comments (4)
LDKNodeMonday/View/Home/OnboardingView.swift (4)
91-127: LGTM! Form-based picker implementation.
The new implementation using Form and NavigationLink provides a more native iOS experience and better accessibility.
132-139:
Enhance security and user experience of seed phrase input.
The TextField implementation should include security-focused modifiers.
TextField(
isFirstTime
? "24 word seed phrase (optional)" : "24 word seed phrase (required)",
text: $viewModel.seedPhrase
)
.textFieldStyle(.roundedBorder)
.submitLabel(.done)
+.keyboardType(.asciiCapable)
+.textContentType(.none)
+.autocorrectionDisabled(true)
+.autocapitalization(.none)
.padding(.horizontal, 50)152-161:
Add loading state to wallet creation button.
The button should provide feedback during wallet creation process.
+@State private var isCreatingWallet = false
+
Button("Create wallet") {
+ isCreatingWallet = true
viewModel.saveSeed()
isFirstTime = false
}
.buttonStyle(
BitcoinFilled(
tintColor: .accent,
isCapsule: true
)
)
+.disabled(isCreatingWallet)
+.overlay {
+ if isCreatingWallet {
+ ProgressView()
+ .tint(.accent)
+ }
+}164-164: Verify accessibility requirements for text size limitations.
The view limits dynamic type size to .accessibility1. Verify if this limitation aligns with your app's accessibility requirements and if users requiring larger text sizes are adequately supported.
| let airdropOptions = ["Receiving Off", "Contacts Only", "Everyone for 10 minutes"] | ||
| @State private var selectedAirdropOption = "Receiving Off" |
There was a problem hiding this comment.
Remove unused airdrop-related code.
The airdropOptions array and selectedAirdropOption state variable appear to be unused in the current implementation.
- let airdropOptions = ["Receiving Off", "Contacts Only", "Everyone for 10 minutes"]
- @State private var selectedAirdropOption = "Receiving Off"
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
LDKNodeMonday/View/Home/OnboardingView.swift (3)
28-40: LGTM! Consider adding accessibility label for the logo image.The layout and styling changes look good. The centered logo with accent color and properly weighted text create a clean hierarchy.
Add an accessibility label to the logo image:
Image(systemName: "bolt.horizontal.fill") .resizable() .aspectRatio(contentMode: .fit) .foregroundColor(.accent) .frame(width: 150, height: 150, alignment: .center) .padding(40) + .accessibilityLabel("Monday Wallet Logo")
Line range hint
47-86: Remove commented code.The old picker implementation is commented out and should be removed to maintain code cleanliness.
- // Default picker style - /* - HStack { - Text("Network") - Spacer() - Picker( - "Network", - selection: $viewModel.selectedNetwork - ) { - Text("Signet").tag(Network.signet) - Text("Testnet").tag(Network.testnet) - } - .pickerStyle(.automatic) - .tint(.accent) - .accessibilityLabel("Select bitcoin network") - } - HStack { - Text("Server") - Spacer() - Picker( - "Esplora server", - selection: $viewModel.selectedURL - ) { - ForEach(viewModel.availableURLs, id: \.self) { url in - Text( - url.replacingOccurrences( - of: "https://", - with: "" - ).replacingOccurrences( - of: "http://", - with: "" - ) - ) - .tag(url) - } - } - .pickerStyle(.automatic) - .tint(.accent) - .accessibilityLabel("Select esplora server") - } - */
165-171: Enhance error alert messaging.The error alert could provide more specific guidance to users.
Alert( - title: Text(viewModel.onboardingViewError?.title ?? "Unknown"), - message: Text(viewModel.onboardingViewError?.detail ?? ""), + title: Text(viewModel.onboardingViewError?.title ?? "Error"), + message: Text(viewModel.onboardingViewError?.detail ?? "An unexpected error occurred. Please try again."), dismissButton: .default(Text("OK")) { viewModel.onboardingViewError = nil } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
LDKNodeMonday/View/Home/OnboardingView.swift(2 hunks)
🔇 Additional comments (2)
LDKNodeMonday/View/Home/OnboardingView.swift (2)
129-136: Enhance security and user experience of seed phrase input.
The TextField needs additional security and usability configurations.
TextField(
isFirstTime
? "24 word seed phrase (optional)" : "24 word seed phrase (required)",
text: $viewModel.seedPhrase
)
.textFieldStyle(.roundedBorder)
.submitLabel(.done)
+.keyboardType(.asciiCapable)
+.textContentType(.none)
+.autocorrectionDisabled(true)
+.autocapitalization(.none)
.padding(.horizontal, 50)149-158: Add loading state to wallet creation button.
The button should provide feedback during wallet creation.
+@State private var isCreatingWallet = false
+
Button("Create wallet") {
+ isCreatingWallet = true
viewModel.saveSeed()
isFirstTime = false
}
.buttonStyle(
BitcoinFilled(
tintColor: .accent,
isCapsule: true
)
)
+.disabled(isCreatingWallet)
+.overlay {
+ if isCreatingWallet {
+ ProgressView()
+ .tint(.accent)
+ }
+}| } | ||
| } | ||
| .tint(.accent) | ||
| .frame(maxHeight: 150) |
There was a problem hiding this comment.
Reconsider fixed height constraint for Form.
The fixed height of 150 might cause content clipping with larger Dynamic Type sizes or when additional options are added.
- .frame(maxHeight: 150)
+ .frame(minHeight: 150)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .frame(maxHeight: 150) | |
| .frame(minHeight: 150) |
|
|
||
| } | ||
| }.dynamicTypeSize(...DynamicTypeSize.accessibility1) // Sets max dynamic size for all Text |
There was a problem hiding this comment.
Reconsider dynamic type size limitation.
The current constraint might make the app less accessible to users who need larger text sizes.
-.dynamicTypeSize(...DynamicTypeSize.accessibility1)
+// Consider testing with larger sizes and adjusting layout instead of limiting text sizeCommittable suggestion skipped: line range outside the PR's diff.
Description
This PR makes changes to the onboarding screen styling and colors.
Before / After light mode / After dark mode
Changelog notice
Checklists
All Submissions:
.swift-formatfileSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
ldk-nodepackage and streamlined package management.