Fix PostDetailsPage's Bookmark and WhichPage position #66
Conversation
thisjustin123
left a comment
thisjustin123
left a comment
There was a problem hiding this comment.
Hmm, something doesn't seem to quite be working on my end...
Make sure to test this on several phone sizes if possible. I think the biggest challenge with fixing this is making sure that this works across different phone sizes. The 170.dp I had before for example may only have been working for my phone.
So even if this 115.dp works for your phone now, it doesn't seem to line up for mine.
thisjustin123
left a comment
thisjustin123
left a comment
There was a problem hiding this comment.
Something still doesn't seem right here lol
Is it working fine on your end? This looks way further in the other direction for me.
|
Is this ready for my review? |
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
This is a ton of files changed for such a small bug fix. I think there is an entire other feature included in this change that is not a part of the PR description. We should remove that from this PR, or just get that other feature merged in first.
There was a problem hiding this comment.
I know you spent a while on this, but I'm sorry to report that it does not work on Pixel 7 Pro. UI fixes like this can genuinely be so tough sometimes. Do you think you can try and find a solution that can work for all devices?
So sorry, I had the wrong branch checked out when I wrote this LMAO. Re-reviewing.
zachseidner1
dismissed
their stale review
oopsies, I was reviewing code that wasn't up to date, I think
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
I think this change prevents me from bookmarking items. Do we need to disable gestures in the sheetDragHandle? Also you may want to look in more into how to have a composable move with the bottom sheet, it feels like there should be a better / less hacky way, although it might just not exist. Maybe the true best solution is to do a custom bottom sheet that we write, but that also might not be very feasible lol.
app/src/main/java/com/cornellappdev/resell/android/ui/screens/pdp/PostDetailPage.kt
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/resell/android/ui/screens/pdp/PostDetailPage.kt
Show resolved
Hide resolved
| Box( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .gesturesDisabled() |
There was a problem hiding this comment.
Still not sure why you're completely disabling gestures here. This seems like it prevents me from bookmarking the item.
Overview
Made the position of the bookmark button and the
WhichPageellipses no longer depend on text height so that they do not overlap with the BottomSheet when the title is long.Details
Due to the lag from update calls, the bookmark button and
WhichPageellipses were noticeably lagging behind the rest of the bottom sheet. Another issue was the cross-device inconsistency that arose from values provided by.onGloballyPositionedbeing slightly off, leading to the same code giving different outcomes.To fix this, I used
BottomSheetScaffold'ssheetDragHandleparameter, which allows customization of the default drag handle. By placing the bookmark button and theWhichPageellipses into this parameter, they no longer lag and are adaptive and consistent.However, one bug that this introduces is that users are not able to swipe through the images if they swipe on the row occupied by the bookmark button. However, I believe this will become significantly less noticeable with the different bookmark button position as in the design.
Related PRs or Issues
Fixes #61