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

CMD + K is not working when the focus is on compose box #3512

Closed
isagoico opened this issue Jun 10, 2021 · 17 comments · Fixed by #3555
Closed

CMD + K is not working when the focus is on compose box #3512

isagoico opened this issue Jun 10, 2021 · 17 comments · Fixed by #3555
Assignees
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2 Reviewing Has a PR in review

Comments

@isagoico
Copy link

isagoico commented Jun 10, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

User should be able to change chats when using the command + k.

Actual Result:

CMD + K is not working when the focus is on compose box.

Action Performed:

  1. Log in to e.cash
  2. Navigate to a conversation
  3. Set the focus to the compose box
  4. Hit CMD + K

Workaround:

User has to manually set the focus on the chat switcher

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

**Version Number:**1.0.66-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

image

Expensify/Expensify Issue URL:

View all open jobs on Upwork


From @iwiznia https://expensify.slack.com/archives/C01GTK53T8Q/p1623271189154800

CMD+K shortcut is not working when the focus is in the chat text box (tested on web in staging, if this is not happening in prod, we should block deploy on it)

Additional info:
This change was caused by upgrading our react-native-web package to 0.15.7.
It appears that when focused on inputs, all existing keybinds no longer work.

@isagoico isagoico added DeployBlockerCash This issue or pull request should block deployment Engineering Daily KSv2 labels Jun 10, 2021
@MelvinBot
Copy link

Triggered auto assignment to @stitesExpensify (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Jun 10, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@stitesExpensify
Copy link
Contributor

Checked out the commit before bumping the react-native-web version and this bug no longer exists. I'm not sure why that is though

@marcaaron
Copy link
Contributor

I am going to need to have someone else look into this for me for #focus. I'd suggest we maybe create a new issue and move forward with the deploy.

@stitesExpensify
Copy link
Contributor

stitesExpensify commented Jun 10, 2021

I think it makes sense to move forward with the deploy, given that we aren't going to roll back react-native-chat. Seems like a good external issue to me as well.

@stitesExpensify
Copy link
Contributor

Removing deploy blocker label and adding external

@stitesExpensify stitesExpensify added External Added to denote the issue can be worked on by a contributor and removed DeployBlockerCash This issue or pull request should block deployment labels Jun 10, 2021
@MelvinBot
Copy link

Triggered auto assignment to @arielgreen (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added the Daily KSv2 label Jun 10, 2021
@stitesExpensify stitesExpensify removed the Hourly KSv2 label Jun 10, 2021
@iwiznia
Copy link
Contributor

iwiznia commented Jun 10, 2021

I disagree about this not being a blocker, I keep sending people messages like brand (because I was trying to switch to the chat with you)

@iwiznia iwiznia added the DeployBlockerCash This issue or pull request should block deployment label Jun 10, 2021
@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Jun 10, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@arielgreen
Copy link
Contributor

I'm a little unclear, should I still get this up on Upwork?

@iwiznia
Copy link
Contributor

iwiznia commented Jun 10, 2021

I don't know. IMO either we should revert the PR that broke this or fix it. @stitesExpensify why do you say we can't revert the PR that broke this?

@roryabraham
Copy link
Contributor

I think I agree with @stitesExpensify on this one – we probably don't want to revert #3215. We won't be doing a prod deploy today anyways, so we have tomorrow to try and get this fixed, so that would be ideal. But otherwise I think we should just :shipit: and not treat this as a deploy blocker.

@trjExpensify
Copy link
Contributor

trjExpensify commented Jun 11, 2021

DB's stay internal because they are hourly priority (unless that's changed), so I'm removing the external label for the time being.

I agree with Rory, let's use tomorrow to assess what the fix is for this. If it's straightforward, let's do it and avoid shipping a regression that has a pretty big impact on everyone's productivity in E.cash. If we end up needing more time, then I think we'll need to remove it as a deploy blocker, so that we can get a new prod build to Apple and unblock N5.

@trjExpensify trjExpensify removed the External Added to denote the issue can be worked on by a contributor label Jun 11, 2021
@Julesssss
Copy link
Contributor

I also don't think this should be a deployBlocker. But I will take a look at the issue today to see if there's a simple fix.

@Julesssss Julesssss self-assigned this Jun 11, 2021
@Julesssss
Copy link
Contributor

No luck. I added some notes on the PR which introduced this library change. Moving onto other deploy blockers.

@Julesssss Julesssss removed their assignment Jun 11, 2021
@trjExpensify
Copy link
Contributor

👋 @HorusGoul found the fix in this thread. Over to you, @roryabraham!

@roryabraham
Copy link
Contributor

Thanks to @HorusGoul the PR to fix this is ready for review

@roryabraham roryabraham added the Reviewing Has a PR in review label Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants