-
Notifications
You must be signed in to change notification settings - Fork 25
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
PM-14175 - Replace vault illustration on create account screen #1102
base: main
Are you sure you want to change the base?
Changes from 10 commits
5ba27d4
f1cfdd1
f5d93f5
683d8af
a6db01f
b0021d3
44475af
c26e169
674c2db
f5313b4
b148a20
24cf0b5
cce820a
754bac4
67d5f8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,10 +13,13 @@ | |
/// The background color to apply to the scroll view. | ||
var backgroundColor: Color | ||
|
||
/// Whether or not to show the scrolling indicators . | ||
var showsIndicators = true | ||
Check warning on line 17 in BitwardenShared/UI/Platform/Application/Appearance/Modifiers/ScrollViewModifier.swift Codecov / codecov/patchBitwardenShared/UI/Platform/Application/Appearance/Modifiers/ScrollViewModifier.swift#L17
|
||
|
||
// MARK: View | ||
|
||
func body(content: Content) -> some View { | ||
ScrollView { | ||
ScrollView(showsIndicators: showsIndicators) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matt-livefront this was also added in the other PR for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also not sure why this isn't being picked up by tests... do you see anything i did wrong herE? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reasoning behind hiding the scroll indicators? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah i should have explained that, my bad. I ran this by Emily and was asked to hide them. |
||
content | ||
.padding(.horizontal, 16) | ||
.padding([.top, .bottom], addVerticalPadding ? 16 : 0) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,11 +118,13 @@ extension View { | |
/// | ||
func scrollView( | ||
addVerticalPadding: Bool = true, | ||
backgroundColor: Color = Asset.Colors.backgroundPrimary.swiftUIColor | ||
backgroundColor: Color = Asset.Colors.backgroundPrimary.swiftUIColor, | ||
showsIndicators: Bool = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐จ Can you add this to the docs above? |
||
) -> some View { | ||
modifier(ScrollViewModifier( | ||
addVerticalPadding: addVerticalPadding, | ||
backgroundColor: backgroundColor | ||
backgroundColor: backgroundColor, | ||
showsIndicators: showsIndicators | ||
)) | ||
} | ||
|
||
|
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 think if we fix the frame to the height of the view, we need to remove the padding that
scrollView
adds and manually add it before the frame modifier. Otherwise, the content size is larger than the scroll bounds and it will scroll slightly.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.
Did i fix this right? my eyes are being deceiving right now... the min spacer height i think is messing with me. Did i add a double top padding here?
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 added a conditional for the top padding and it looks better to me