Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: prepare for new search modal #11033
feat: prepare for new search modal #11033
Changes from 10 commits
8c9105b
49210fe
6f4ef68
e729936
6fb12c2
b54b323
4b5da4d
067c8d4
58adac5
da0b37f
2be954f
e893df0
e090943
4710647
1d8414b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
just out of curiosity: why do we need
pointerEvents="none"
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.
here, this was my solution to avoid building a search input button that looks like the input. So I made the input clickable, but to avoid focusing inside it, I wrapped it with a flex so it doesn't send touch events to its children. And voila, a search input button that's clickable but not focusable
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.
That's a great idea! 🌟
A short comment in the code to explain what this Flex container does could be good..
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 don't fully understand why there is a GlobalSearchInputModal and a SearchModalScreen 🤔
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.
good point - I am actually still trying to figure out the best way to present this modal.
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.
Overlay, modal or a modally presented screen.
absolute
positioned view covering everythingI am checking all options currently again and I am leaning towards the first solution. I know it's what we spoke about during the refinement but I wanted to also explore other approaches
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.
Thank you very much for explaining 🙏
Intuitively, the first solution sounds like the best solution to me as well.
If using an
absolute
positioned view covering the home screen does not work, an alternative could be to hide the screen content (e.g., by setting its height to 0).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 found a way, I did exactly that but without needing to add a store value to show/hide the overlay view on both search and home views. And also without need for prop drilling.
I noticed that we are using
react-native-portal
as a peer dependency inyarn.lock
but we were not importing it anywhere. So I specified that and used it. The API is so straightforward and it worked great