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
Add Context menu with Pin option to LHN #16079
Add Context menu with Pin option to LHN #16079
Changes from 18 commits
fc9aec9
5bbf1db
ffaf634
8789f75
d3befa3
28079fe
c3c45ce
a6e1aec
6b26871
cc83d47
6d65fb8
96c8256
eafa87f
310ae83
315fbc6
7ef10b8
5803c47
9f4349d
233ae47
223a550
294ed23
95de7c2
d4c619e
7263848
0c827dc
b554112
fb6c9b4
cc01e7e
8c45c6f
7f61b2a
3775be7
a0b72c6
f3c9046
c1a93de
beaa0e0
698b876
9e7ce9b
9cab709
f125e5d
fddded2
137631e
a433efb
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.
Let's use same pattern as others. What's the difference (pros/cons) between
Hoverable
insidePressableWithSecondaryInteraction
andPressableWithSecondaryInteraction
insideHoverable
?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 have
hovered
available for styling purposes. I think that's the main reason I have it this wayThere 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.
Touchable components will be deprecated - #14589.
Let's keep using
Pressable
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 need this to be a
TouchableOpacity
for the ReportActions. It is not a "new" one, we are just moving it around. Once we decide how we want to handle those, then we'll replace it 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.
Would you mind elaborating current issue with
Pressable
? so why you are forced to useTouchableOpacity
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.
@0xmiroslav does this blurb from the PR description answer your question?
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.
ReportActions currently use a TouchableOpacity, and the current UI is based on that. I'm moving that TouchableOpacity to the PressableWithSecondaryInteraction component (which currently surrounds the TouchableOpacity for ReportActions) so that it works correctly in all cases. Using Pressable here would mess the UI.
We are currently designing/developing a new component to replace TouchableOpacity. When that happens, it'll simply get replaced here too.
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.
Sorry but I am still not convinced about this change. We're now actively deprecating all Touchable components to use Pressable.
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.
Ah, yes, I changed this back in index.js, but forgot to do so for native. That's also what's causing the error.