DON-1089: Fixing a scroll issue in BPKCalendar #2158
Conversation
yurareutskiy
force-pushed
the
donburi/DON-1089_fix_scroll_issue_in_calendar
branch
2 times, most recently
from
ee4a5c6 to
55375df
Compare
| ) | ||
| .frame(height: headerHeight) | ||
| .modifier(ReadSizeModifier { | ||
| headerHeight = max($0.height, headerHeight ?? 0) |
There was a problem hiding this comment.
How about having the initial headerHeight as 0 instead of optional nil ?
There was a problem hiding this comment.
Answered to that below
| @@ -32,7 +33,7 @@ struct CalendarContainer<MonthContent: View>: View { | |||
| LazyVStack(spacing: BPKSpacing.none) { | |||
| ForEach(0...monthsToShow, id: \.self) { monthIndex in | |||
| let firstDayOfMonth = firstDayOf(monthIndex: monthIndex) | |||
| monthContent(firstDayOfMonth) | |||
| monthContent(firstDayOfMonth, $accessoryViewHeight) | |||
There was a problem hiding this comment.
Do we still need accessoryViewHeight for monthContent ?
There was a problem hiding this comment.
so, the idea is to share between each month view the accessory view height, so it wouldn't need to calculate it each time
this should help with the consistency
the problem could be because the magnifier icon and the text have different heights (15.6 vs 16.0), so in my understanding, if one month has only one magnifiers, then when it happened to display a text, then it would need to recalculate the heights
and to avoid that issue I thought about this sharable height value, so every component already would aware of the target height
however (!), I've checked the latest changes with removing that sharable height and keeping it as a state (as it was before), and I haven't noticed anything wrong with that after some tests
what do you think?
There was a problem hiding this comment.
I think we can keep it as it was before. It seems other fixes helped us to reduce/escape the flakiness on this height. I couldn't reproduce the issue again.
There was a problem hiding this comment.
I'm not sure caching a height is a good solution here, it should be left to SwiftUI to calculate on real time based on constraints, I think there is something else happening here, perhaps something we're doing wrong when reusing month cells or similar? setting a frame for things should almost never be the solution unless specifically due to a design reason
There was a problem hiding this comment.
@frugoman
This change has been reverted ✅ It was just an open comment.
| @@ -28,7 +40,8 @@ struct CalendarMonthGrid< | |||
| let calendar: Calendar | |||
| let validRange: ClosedRange<Date> | |||
|
|
|||
| @State private var dayCellHeight: CGFloat = 0 | |||
| @State private var dayCellHeight: CGFloat? | |||
| @Binding var accessoryViewHeight: CGFloat? | |||
There was a problem hiding this comment.
How about having the initial accessoryViewHeight and dayCellHeight as 0 instead of optional nil ?
There was a problem hiding this comment.
we shouldn't set 0s here
it's important to set nil as an initial value, so swiftui could calculate it correctly and return the correct view when we call .frame() modifier
I tried to set to 0, it didn't work
that's an example
ScreenRecording_02-06-2025.13-26-57_1.MP4
There was a problem hiding this comment.
That makes sense as we want to get the size behaviour from the view for the first time.
https://developer.apple.com/documentation/swiftui/view/frame(width:height:alignment:)
Thank you for checking.
yurareutskiy
force-pushed
the
donburi/DON-1089_fix_scroll_issue_in_calendar
branch
from
920dd0c to
909b47b
Compare
|
Fixed in #2167 |
DON-1089
When scrolling back upwards sometimes from a future month to a previous month the header jumps down and causes the overlapping issue on the right
The solution was to calculate heights and keep them cached. Also make all cells in a LazyVGrid contained in one array, so just one ForEach would be used (apparently, there is an issue with using multiple ForEach)
More info:
https://skyscanner.atlassian.net/wiki/x/Ty9rTQ
Remember to include the following changes:
README.mdBackpack.hheader fileIf you are curious about how we review, please read through the code review guidelines