-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Hide mobile Search nav button + status tabs on scrollDown, but reveal on scrollUp #48258
base: main
Are you sure you want to change the base?
Hide mobile Search nav button + status tabs on scrollDown, but reveal on scrollUp #48258
Conversation
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.
generally LGTM, works great! 👀
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.
Hey very nice work both on animation and refactoring. SearchPageHeader feels much simpler, and also Search
component also looks simpler.
My only real problem is with SearchPageBottom
because it became harder to read.
Most of my comments are minor naming issues, LGTM otherwise 👍
isCustomQuery: boolean; | ||
setOfflineModalOpen?: () => void; | ||
setDownloadErrorModalOpen?: () => void; | ||
data?: TransactionListItemType[] | ReportListItemType[]; |
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.
great job removing so many props! 👏
hash={queryJSON.hash} | ||
isCustomQuery={isCustomQuery} | ||
/> | ||
)} |
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.
Do you think its possible to somehow simplify this and reduce the amount of conditional rendering inside this return?
We have conditional rendering for TopBar
+ SearchTypeMenu
, then extra condition for SearchStatusBar
and all that is exclusive with SearchPageHeader
rendering.
It's very hard to understand which parts of the UI will be rendered.
Some ideas:
- early returns?
- saving parts of JSX to variables?
- small helper components declared within this file?
I think you could try to go over this code again and see if anything can be improved.
@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Running a test build for easier testing... |
This comment has been minimized.
This comment has been minimized.
Any idea how to fix the iOS build above so we can test? |
I merged main and the error should be solved(ruby-version file was missing, but it was reintroduced after merge). Can you try again? |
Will do! |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
I think this is feeling pretty darn good! |
I messed something up with merging. Will fix it in 5 minutes. |
I addressed Jon's comment above. Let's see what Danny thinks. I don't mind how it is working now though, I could totally get down with it's current behavior. |
@dannymcclain we need your opinion here! 😄 |
🫣 I kinda like the quick appear. But I will also add that I don't think there's a right answer here—both behaviors are totally totally valid. Like Shawn noted, I've seen and used both behaviors out in the wild and neither are "bad". @dubielzyk-expensify if you feel strongly about changing the incoming animation, I am totally good with that. I don't feel insanely strongly because like I said, I think both approaches are good. If we want to get this behavior merged as is and come back to it, that's totally fine too. What I like about the quick appear is that (for me personally) I'm likely scrolling up to get to those options, so getting them in front of me quickly is nice. I think I've also mentioned this before, but the quick appear also (again, for me) signals that this menu is being overlaid on top of the list and is not part of the list. So I know there's more expenses "under" it if I keep scrolling up. |
I'm happy to move forward with this 😄 |
Nice! Then this is ready for your review @rayane-djouah |
Nice! It's looking really good. @rayane-djouah let's start reviewing this one |
Will review this first thing in my morning |
@@ -27,18 +27,20 @@ function SelectionList<TItem extends ListItem>(props: BaseSelectionListProps<TIt | |||
}; | |||
}, []); | |||
|
|||
const defaultOnScroll = () => { |
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.
const defaultOnScroll = () => { | |
const dismissKeyboard = () => { |
@@ -3610,6 +3610,14 @@ const styles = (theme: ThemeColors) => | |||
fontWeight: FontUtils.fontWeight.bold, | |||
}, | |||
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Let's rename the file to SearchSelectionModeHeader.tsx
import SearchTypeMenu from './SearchTypeMenu'; | ||
|
||
const TOO_CLOSE_TO_TOP_DISTANCE = 5; | ||
const TOO_CLOSE_TO_BOTTOM_DISTNACE = 10; |
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.
const TOO_CLOSE_TO_BOTTOM_DISTNACE = 10; | |
const TOO_CLOSE_TO_BOTTOM_DISTANCE = 10; |
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.
We still have this typo 😄
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.
Bug on small screens: Selected multiple expenses and clicked "delete," but the delete confirmation modal did not appear.
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-19.at.23.02.42.mp4
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.
fixed your comments @rayane-djouah! |
} | ||
Keyboard.dismiss(); | ||
}} | ||
onScroll={onScroll ?? defaultOnScroll} |
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.
@SzymczakJ Can we use dismissKeyboard in onScrollBeginDrag
and our custom onScroll in onScroll
?
import SearchTypeMenu from './SearchTypeMenu'; | ||
|
||
const TOO_CLOSE_TO_TOP_DISTANCE = 5; | ||
const TOO_CLOSE_TO_BOTTOM_DISTNACE = 10; |
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.
We still have this typo 😄
Bug: Status bar is not scrollable on the Android native app |
We've agreed upon second version here #48019 (comment) |
Fixed your comments @rayane-djouah! |
Details
This PR adds an animation that hides mobile Search nav bar on scroll down and reveals it on scrollUp. Please note that this feature is only meant to work on mobile screens. Also when user doesn't have enough expenses, the bar won't hide.
Fixed Issues
$ #48019
PROPOSAL:
Tests
Offline tests
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
androidweb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
iosweb.mov
MacOS: Chrome / Safari
MacOS: Desktop