Did minor UI fixes #5
Conversation
VilemKurz
force-pushed
the
screenshot-status-segregation
branch
3 times, most recently
from
bfefedb to
639824f
Compare
Packages/Sources/ConnectWalletFeature/ConnectWalletFeature.swift
Outdated
Show resolved
Hide resolved
Packages/Sources/ConnectWalletFeature/ConnectWalletFeature.swift
Outdated
Show resolved
Hide resolved
hilalalhakani
force-pushed
the
minorUIFixes
branch
3 times, most recently
from
5574235 to
85666c2
Compare
hilalalhakani
force-pushed
the
minorUIFixes
branch
from
85666c2 to
4d618cb
Compare
Packages/Tests/SnapshotsTests/ConnectWalletSnapshotsTests.swift
Outdated
Show resolved
Hide resolved
Packages/Tests/SnapshotsTests/ConnectWalletSnapshotsTests.swift
Outdated
Show resolved
Hide resolved
|
I will rebase this into main. |
There was a problem hiding this comment.
One more comment regarding the image precision - it seemed that only five screens were failing recently, in ConnectWalletSnapshotsTests? Lowering the percentage is fine and it works, but please be aware that XCTExpectFailure can warn you as soon as the issue is fixed in future snapshot testing lib updates, or screen changes - so that you can move back to normal 0.98. Writing just so that you are aware of this option, do as you prefer.
| Text("This page will open another application.", bundle: .module) | ||
| .multilineTextAlignment(.center) | ||
| .foregroundStyle(Color.neutral2) | ||
| .font(Font(FontName.poppinsRegular, size: 12)) | ||
| .frame(width: 215) |
There was a problem hiding this comment.
This line can be removed. The width of the text should be driven by the VStack width, which is driven by padding 32 (this is fine as you have it)
| .background( | ||
| Color.neutral8 | ||
| Color.connectWalletAlertBackground | ||
| .clipShape(RoundedRectangle(cornerRadius: 32)) |
There was a problem hiding this comment.
There is one spacing missing - the padding of 48 between the alert background and leading/trailing layout margins. The alert is too wide.
| .stroke(Color.neutral6) | ||
| ) | ||
| .frame(minHeight: 40) | ||
| Button( |
There was a problem hiding this comment.
This method is way too long, which makes you add vertical spaces to logically divide the code. You can introduce this method func makeButton(foregroundStyle:tint:isBordered:label:action) -> Button to dedupe and simplify the code.
| @@ -207,50 +207,51 @@ struct PopUpModifier: ViewModifier { | |||
| .transition(.opacity.animation(.easeInOut)) | |||
There was a problem hiding this comment.
Is the environment colorScheme still used?
| foregroundStyle: .neutral2, | ||
| tint: .clear, | ||
| label: { | ||
| Text("Cancel") |
There was a problem hiding this comment.
You can also dedupe the text
I made some subtle adjustments to the UI