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
fix: issue 23908 #27190
fix: issue 23908 #27190
Changes from 17 commits
002241b
920d7c8
09fe747
3951fd7
d0e69fe
1b7e45d
b0dcc75
164f88d
b887abd
77da6ed
418ddd4
b79780d
b2f4e3a
98d573e
3a246e6
b96510d
a74b2b2
e4a381f
97aaa26
c24df9c
4e9083f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
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.
As this effect depends on temporary data it is not synced with the lifecycle of other components creating inconsistencies between states of components. It caused #28029
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.
NAB. Do we need to make this keyboard aware? Will this introduce any delay?
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 think so. I think showing composer after keyboard is closed is good. If keyboard isn't visible, this would show composer immediately
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 think this may be causing a regression. Can we remove the condition here? Currently if we delete a message the focus is not set back to the main composer but it should
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 think that solution won't work. We still need to find a way to fix the regression though
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 think we should focus the main composer if the deleted message is active. Please let me know if I am 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.
That's correct but apparently this is not the behavior
Screen.Recording.2023-09-14.at.5.29.25.PM.mov
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.
@s77rt
There were some changes recently. This is due to this PR. Let me find a way
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 fixed the bug of
isActiveReportAction
. Now this works fine as expectedThere 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 guess this is resolved, right? Because I too agree the expected behaviour is that composer should get the focus.
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.
yes
#27190 (comment)
cc @s77rt
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.
ok, thanks.
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.
Shouldn't we do this on
onBlur
?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.
No. We should keep active report action until another draft gets focused. This wasn't already confirmed?
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.
@s-alves10 This is not resolved yet. Can we do this any other way?
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.
Clear active report action when another action gets focused
We shouldn't clear active action on blur because blurred action can be active.
I think we confirmed this in the proposal
Note: I guess we can clear active in
onModalHide
ofEmojiPickerButton
forEmojiPicker
, and in confirm/cancel callback ofshowDeleteModal
forReportActionContextMenu
. Please let me know your thoughtsThere 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.
@s77rt
I think the new approach can cause a regression.
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 see. I don't think this is a blocker for now. Will need to test though