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.

Sign up for GitHub

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

Jump to bottom

UI - onboarding styling and colors #131

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

UI - onboarding styling and colors #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8ca66a9
Fix ldk node package issue
danielnordh Oct 25, 2024
ea5109e
Update Onboardingview, logo, styling
danielnordh Oct 25, 2024
235faa0
Use Monday name and Bolt image
danielnordh Oct 25, 2024
d206422
Format code
danielnordh Oct 25, 2024
b330dd1
Set max dynamic type size
danielnordh Nov 27, 2024
6fcd2ff
WIP: .navigationlink style, use apple text style
danielnordh Nov 27, 2024
ffae221
Remove unused stuff
danielnordh Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions LDKNodeMonday.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
objects = {

/* Begin PBXBuildFile section */
65875A932CCB9809000D3E70 /* LDKNode in Frameworks */ = {isa = PBXBuildFile; productRef = 65875A922CCB9809000D3E70 /* LDKNode */; };
AE00550E2B479EF000100797 /* OnboardingView.swift in Sources */ = {isa = PBXBuildFile; fileRef = AE00550D2B479EF000100797 /* OnboardingView.swift */; };
AE0055102B479F1100100797 /* OnboardingViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = AE00550F2B479F1100100797 /* OnboardingViewModel.swift */; };
AE0055122B47A65700100797 /* Color+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = AE0055112B47A65700100797 /* Color+Extensions.swift */; };
Expand Down Expand Up @@ -86,7 +87,6 @@
AEE8FDDD29F855E700406DD9 /* String+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = AEE8FDDC29F855E700406DD9 /* String+Extensions.swift */; };
AEE8FDDF29F8579600406DD9 /* Date+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = AEE8FDDE29F8579600406DD9 /* Date+Extensions.swift */; };
AEF66F522BA4B11A00FA15F9 /* PaymentKind+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = AEF66F512BA4B11A00FA15F9 /* PaymentKind+Extensions.swift */; };
AEFA825E2CC15C7C0095B0BA /* LDKNode in Frameworks */ = {isa = PBXBuildFile; productRef = AEFA825D2CC15C7C0095B0BA /* LDKNode */; };
AEFAF2922A433F6F001128DF /* LightningPaymentStatus.swift in Sources */ = {isa = PBXBuildFile; fileRef = AEFAF2912A433F6F001128DF /* LightningPaymentStatus.swift */; };
/* End PBXBuildFile section */

Expand Down Expand Up @@ -173,7 +173,7 @@
isa = PBXFrameworksBuildPhase;
buildActionMask = 2147483647;
files = (
AEFA825E2CC15C7C0095B0BA /* LDKNode in Frameworks */,
65875A932CCB9809000D3E70 /* LDKNode in Frameworks */,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

AE060C382C051B59006724F1 /* LDKNode in Frameworks */,
AE1D9BEC2B2A1FFD00620748 /* BitcoinUI in Frameworks */,
AEE5B7652A09B1FC001E5E59 /* CodeScanner in Frameworks */,
Expand Down Expand Up @@ -548,7 +548,7 @@
AE7C4A072B406D590061189D /* SimpleToast */,
AE060C372C051B59006724F1 /* LDKNode */,
AE80C2042C4AB5E4006E7193 /* LDKNode */,
AEFA825D2CC15C7C0095B0BA /* LDKNode */,
65875A922CCB9809000D3E70 /* LDKNode */,
);
productName = LDKNodeMonday;
productReference = AE17E8D629A402E30058C9C9 /* LDKNodeMonday.app */;
Expand Down Expand Up @@ -583,7 +583,7 @@
AE01C5AE2AB3BEED00F28C7E /* XCRemoteSwiftPackageReference "KeychainAccess" */,
AE1D9BEA2B2A1FFD00620748 /* XCRemoteSwiftPackageReference "BitcoinUI" */,
AE7C4A062B406D590061189D /* XCRemoteSwiftPackageReference "SimpleToast" */,
AEFA825C2CC15C7C0095B0BA /* XCRemoteSwiftPackageReference "ldk-node" */,
65875A912CCB9809000D3E70 /* XCRemoteSwiftPackageReference "ldk-node" */,
);
productRefGroup = AE17E8D729A402E30058C9C9 /* Products */;
projectDirPath = "";
Expand Down Expand Up @@ -894,6 +894,14 @@
/* End XCConfigurationList section */

/* Begin XCRemoteSwiftPackageReference section */
65875A912CCB9809000D3E70 /* XCRemoteSwiftPackageReference "ldk-node" */ = {
isa = XCRemoteSwiftPackageReference;
repositoryURL = "https://github.com/lightningdevkit/ldk-node.git";
requirement = {
kind = exactVersion;
version = 0.4.0;
};
};
Comment on lines +897 to +904
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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";
};
};

AE01C5AE2AB3BEED00F28C7E /* XCRemoteSwiftPackageReference "KeychainAccess" */ = {
isa = XCRemoteSwiftPackageReference;
repositoryURL = "https://github.com/kishikawakatsumi/KeychainAccess.git";
Expand Down Expand Up @@ -926,17 +934,14 @@
version = 2.0.0;
};
};
AEFA825C2CC15C7C0095B0BA /* XCRemoteSwiftPackageReference "ldk-node" */ = {
isa = XCRemoteSwiftPackageReference;
repositoryURL = "https://github.com/lightningdevkit/ldk-node.git";
requirement = {
kind = exactVersion;
version = 0.4.0;
};
};
/* End XCRemoteSwiftPackageReference section */

/* Begin XCSwiftPackageProductDependency section */
65875A922CCB9809000D3E70 /* LDKNode */ = {
isa = XCSwiftPackageProductDependency;
package = 65875A912CCB9809000D3E70 /* XCRemoteSwiftPackageReference "ldk-node" */;
productName = LDKNode;
};
AE01C5AF2AB3BEED00F28C7E /* KeychainAccess */ = {
isa = XCSwiftPackageProductDependency;
package = AE01C5AE2AB3BEED00F28C7E /* XCRemoteSwiftPackageReference "KeychainAccess" */;
Expand Down Expand Up @@ -969,11 +974,6 @@
package = AEE5B7632A09B1FC001E5E59 /* XCRemoteSwiftPackageReference "CodeScanner" */;
productName = CodeScanner;
};
AEFA825D2CC15C7C0095B0BA /* LDKNode */ = {
isa = XCSwiftPackageProductDependency;
package = AEFA825C2CC15C7C0095B0BA /* XCRemoteSwiftPackageReference "ldk-node" */;
productName = LDKNode;
};
/* End XCSwiftPackageProductDependency section */
};
rootObject = AE17E8CE29A402E30058C9C9 /* Project object */;
Expand Down
18 changes: 14 additions & 4 deletions LDKNodeMonday/Assets.xcassets/AccentColor.colorset/Contents.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@
"colors" : [
{
"color" : {
"platform" : "universal",
"reference" : "systemBlueColor"
"color-space" : "display-p3",
"components" : {
"alpha" : "1.000",
"blue" : "0.894",
"green" : "0.188",
"red" : "0.111"
}
},
"idiom" : "universal"
},
Expand All @@ -15,8 +20,13 @@
}
],
"color" : {
"platform" : "universal",
"reference" : "systemBlueColor"
"color-space" : "display-p3",
"components" : {
"alpha" : "1.000",
"blue" : "0xCD",
"green" : "0xF3",
"red" : "0x77"
}
Comment on lines +23 to +29
Copy link

Choose a reason for hiding this comment

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

💡 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:

  1. Has a relative luminance that may not provide sufficient contrast with light text
  2. Uses excessive brightness (especially green at 0xF3) that could cause visual discomfort in dark mode
  3. 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:

  1. The color provides sufficient contrast with text
  2. The brightness level is comfortable for dark mode viewing
  3. 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

},
"idiom" : "universal"
}
Expand Down
187 changes: 105 additions & 82 deletions LDKNodeMonday/View/Home/OnboardingView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,55 +25,45 @@ struct OnboardingView: View {

Spacer()

VStack(spacing: 25) {
VStack(spacing: -5) {
Image(systemName: "bolt.horizontal.fill")
.resizable()
.aspectRatio(contentMode: .fit)
.foregroundColor(Color(hex: "77F3CD"))
.frame(width: 100, height: 100)
Text("Monday Wallet")
.foregroundColor(Color(hex: "77F3CD"))
.font(.largeTitle)
.fontDesign(.monospaced)
.fontWeight(.light)
.multilineTextAlignment(.center)
.lineLimit(1)
.minimumScaleFactor(0.5)
}
Text("LDK Node Lightning Wallet")
.foregroundColor(Color(hex: "77F3CD"))
.fontDesign(.monospaced)
VStack {
Image(systemName: "bolt.horizontal.fill")
.resizable()
.aspectRatio(contentMode: .fit)
.foregroundColor(.accent)
.frame(width: 150, height: 150, alignment: .center)
.padding(40)
Text("Monday Wallet")
.font(.largeTitle .weight(.semibold))
Text("An example bitcoin wallet\npowered by LDK Node")
.font(.body)
.multilineTextAlignment(.center)
.lineLimit(1)
.minimumScaleFactor(0.5)
.padding(.horizontal, 10)
.padding(.vertical, 10)
.background(
RoundedRectangle(cornerRadius: 5)
.foregroundColor(Color(uiColor: .label))
)
.fixedSize(horizontal: false, vertical: true)
}
.padding(.vertical, 10)
.padding(.horizontal, 50)

VStack {
Spacer()

VStack {
NavigationStack {
// Default picker style
/*
HStack {
Text("Network")
Spacer()
Picker(
"Network",
selection: $viewModel.selectedNetwork
) {
Text("Bitcoin").tag(Network.bitcoin)
Text("Testnet").tag(Network.testnet)
Text("Signet").tag(Network.signet)
Text("Regtest").tag(Network.regtest)
Text("Testnet").tag(Network.testnet)
}
.pickerStyle(.automatic)
.tint(viewModel.buttonColor)

.tint(.accent)
.accessibilityLabel("Select bitcoin network")
}
HStack {
Text("Server")
Spacer()
Picker(
"Esplora Server",
"Esplora server",
selection: $viewModel.selectedURL
) {
ForEach(viewModel.availableURLs, id: \.self) { url in
Expand All @@ -90,63 +80,96 @@ struct OnboardingView: View {
}
}
.pickerStyle(.automatic)
.tint(viewModel.buttonColor)

.tint(.accent)
.accessibilityLabel("Select esplora server")
}

*/
// NavigationLink picker style
Form {
Section() {
Picker(
"Network",
selection: $viewModel.selectedNetwork
) {
Text("Signet").tag(Network.signet)
Text("Testnet").tag(Network.testnet)
}
.pickerStyle(.navigationLink)
.accessibilityLabel("Select bitcoin network")
.scrollContentBackground(.hidden)
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(.navigationLink)
.accessibilityLabel("Select esplora server")
.scrollContentBackground(.hidden)
}
}
.tint(.accent)
.frame(maxHeight: 150)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
.frame(maxHeight: 150)
.frame(minHeight: 150)

.scrollContentBackground(.hidden)
}
.padding()

VStack {
TextField(
isFirstTime
? "24 word Seed Phrase (Optional)" : "24 word Seed Phrase (Required)",
text: $viewModel.seedPhrase
.padding(.horizontal, 20)

// Textfield for importing wallet
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
Comment on lines +129 to +143
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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

)
.textFieldStyle(RoundedBorderTextFieldStyle())
.submitLabel(.done)
.padding(.horizontal, 40)
if viewModel.seedPhraseArray != [] {
SeedPhraseView(
words: viewModel.seedPhraseArray,
preferredWordsPerRow: 2,
usePaging: true,
wordsPerPage: 6
)
}
}
.padding()

Spacer()

Button {
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
)
)
Comment on lines +149 to +158
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

.padding()

}
}.dynamicTypeSize(...DynamicTypeSize.accessibility1) // Sets max dynamic size for all Text
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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 size

Committable suggestion skipped: line range outside the PR's diff.


}
.alert(isPresented: $showingOnboardingViewErrorAlert) {
Alert(
title: Text(viewModel.onboardingViewError?.title ?? "Unknown"),
message: Text(viewModel.onboardingViewError?.detail ?? ""),
dismissButton: .default(Text("OK")) {
viewModel.onboardingViewError = nil
}
)
}
}.padding(.bottom, 20)
.alert(isPresented: $showingOnboardingViewErrorAlert) {
Alert(
title: Text(viewModel.onboardingViewError?.title ?? "Unknown"),
message: Text(viewModel.onboardingViewError?.detail ?? ""),
dismissButton: .default(Text("OK")) {
viewModel.onboardingViewError = nil
}
)
}

}
}
Expand Down