Skip to content
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

[Improvement] Populate cursor to compose box after chat has been selected #1513

Closed
kadiealexander opened this issue Feb 19, 2021 · 15 comments · Fixed by #2050
Closed

[Improvement] Populate cursor to compose box after chat has been selected #1513

kadiealexander opened this issue Feb 19, 2021 · 15 comments · Fixed by #2050
Assignees

Comments

@kadiealexander
Copy link
Contributor

kadiealexander commented Feb 19, 2021

If you haven’t already, check out our contributing guidelines for onboarding!


Platform - version: Desktop v.435

Action Performed (reproducible steps):

  • command+k to access new Right Hand Nav search
  • select user

Expected Result:
Once user is selected, cursor is selected in the compose box and I should be able to immediately start typing text

Actual Result:
The cursor didn't populate, you need to click the compose box to start typing.

@kadiealexander
Copy link
Contributor Author

@aliabbasmalik8
Copy link
Contributor

aliabbasmalik8 commented Feb 19, 2021

Here is the solution to this issue,
I tested it on the web and it's worked properly
file-path: src/pages/home/report/ReportActionCompose

image

@tgolen
Copy link
Contributor

tgolen commented Feb 23, 2021

@aliabbasmalik8 Thank you for the proposal. Could you please give a little more detail on why you found the text box is not getting focused in the first place? I'm pretty sure this is a regression from previous versions. Could you also explain how your proposal fixes it? Is the setTimeout() necessary or are there other solutions?

@aliabbasmalik8
Copy link
Contributor

aliabbasmalik8 commented Feb 23, 2021

@tgolen
when we pressed command+k to access the new Right Hand Nav search then basically route changed so if we clicked the currently active user then the component did not rerender so in this case, we need to handle isFocused in componentDidUpdate lifecycle

if (this.props.redirectTo && this.props.redirectTo !== prevProps.redirectTo) {
            this.setIsFocused(this.props.redirectTo !== '/search');
        }

when we clicked on other user from the Right Hand Nav Search then route changed, as well as ReportActionCompose component rerenderand at this time onBlur method also called right after onFocus method called. so only working solution of this issue is add setTimeout(() => this.setIsFocused(true), 0); in componentDidMount lifecycle.

@tgolen
Copy link
Contributor

tgolen commented Feb 24, 2021

OK, thank you for a little more details. In the solution for componentDidUpdate(), where does this.props.redirectTo come from? I don't see it listed in the propTypes anywhere.

ReportActionCompose component rerenderand at this time onBlur method also called right after onFocus method called

Can you link to the code where this blur is happening? I'm not seeing it and just want to double-check. My thought is that we should not be calling focus() -> blur() -> focus(). It seems better that we just not call blur() in that case.

@aliabbasmalik8
Copy link
Contributor

aliabbasmalik8 commented Feb 24, 2021

We need to add redirectTo in the below position

image

on click outside of the message text box then we need to call blur() function
Here is the screen recording.
https://recordit.co/tAHb0oW4pS

@SameeraMadushan
Copy link
Contributor

SameeraMadushan commented Mar 1, 2021

Hi @tgolen

Issue

Here the problem is in the Modal rendering.
After navigation changes to search, new/chat, or new/group it will load the modal.
When again come back to /r/reportID it will take milliseconds to render the animation.
In the mean time componentDidMount() in TextInputFocusable will execute and this.focusInput() will also execute.
But Input won't be able to stay focus because the component is not focused, and it will blur automatically.

Solution

Inside TextInputFocusable's componentDidMount(), I will move this.focusInput() statement inside setTimeout to add a minimum milliseconds of delay.

        setTimeout(() => {
            this.focusInput();
        }, 200);

So it will focus the input correctly.

Thanks,
Sameera

@tgolen
Copy link
Contributor

tgolen commented Mar 1, 2021

Hi @SameeraMadushan,

Thanks for your proposal. I understand the 0 value for setTimeout() but I'd definitely like to avoid it if there is any way possible. Is it because of the animation happening that causes the issue? If the animation is disabled, does the issue go away? Is there any way to know when the animation is done so that we know it's safe to focus the input?

@SameeraMadushan
Copy link
Contributor

SameeraMadushan commented Mar 1, 2021

@tgolen Yes.
In the react-native-modal we have isVisible prop.
When we are using that it will work together with animation props. (animationIn, animationOut)

Fix

If I render RightDockedModal with the same condition used to isVisible then the Text input focus works fine. (check the image)

image

The reason is when we use condition here, It will hide the modal without waiting for the animation to close the modal.
If we use this method to show/hide modal, no need to use setTimeout.

(We need to have isVisible prop also in the modal because it's required for react-native-modal)

@tgolen
Copy link
Contributor

tgolen commented Mar 1, 2021

Great, thanks for the info. animationOut would be great to use. Let's go with that!

@aliabbasmalik8
Copy link
Contributor

aliabbasmalik8 commented Mar 2, 2021

@tgolen
if we clicked the currently active user from modal then the component did not rerender so in this case, we need to handle isFocused in componentDidUpdate lifecycle, and in this case, animationOut also worked

if (this.props.redirectTo && this.props.redirectTo !== prevProps.redirectTo) {
      this.setIsFocused(this.props.redirectTo !== '/search');
}

But when we clicked some other user rather than an active user that case we can handle isFocused with settimeout or with conditional modal render but in both way, animationOut will not work because, after click on another user row, route change and modal disappeared before animationOut work

We can handle animationOut in this case using global state using react-native-onyx( already used component) and yeah app already have isSidebarShown state but that used all type of sidebar and on web, we have 2 sidebars at a time when we open modal

  1. Left Side Bar
  2. Modal also handle with sidebar state

We can add a sidebar type to handle this case or can create a new state for the modal.

NOTE: currently animationOut not working in 2nd case
We also need to add shouldHighlight && this.textInput.focus(); in setIsFocused function.
Looking for your suggestion
Thanks

@kadiealexander
Copy link
Contributor Author

@aliabbasmalik8 thank you for this proposal! We would like to proceed with this as a fix. I have sent the contract in Upwork for you to get started. 😊

@SameeraMadushan
Copy link
Contributor

isFocused state only used to change the style of the text input.
Not to focus the Text Input. :)

@tgolen
Copy link
Contributor

tgolen commented Mar 2, 2021

Hi, thank you for explaining that second case. I think I understand it.

I like the idea for adding a modal state into Onyx, something like isModalShown.

  • Whenever animationIn is triggered, isModalShown = true
  • Whenever animationOut is triggered, isModalShown = false
  • Whenever isModalShown goes from true to false, then the chat input receives focus (this would be handled in componentDidUpdate()

Is this mostly what you had in mind?

@aliabbasmalik8
Copy link
Contributor

@tgolen
Yes, Sure will do accordingly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants