Repository navigation
[Hold for payment Aug 3] KeyboadShortcut is broken #4028
Description
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
- Open Search with
Cmd + K - Open New Group with
Cmd + Shift + K - Click outside New Group popup to close it.
- Press
Escapeto close Search.
Expected Result:
Escape should close the Search page.
Actual Result:
Escape doesn't close the Search page.
Workaround:
The user can click outside Search page/ click on the close button.
Platform:
Where is this issue occurring?
Web ✔️
iOS
Android
Desktop App ✔️
Mobile Web
Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
issueDemo.mp4
Expensify/Expensify Issue URL:
Upwork job: https://www.upwork.com/jobs/~011663eb89a2b43197
Activity
MelvinBot commentedon Jul 14, 2021
Triggered auto assignment to @puneetlath (
AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.rushatgabhane commentedon Jul 14, 2021
Why is this issue occuring?
The issue is occuring because Push/Pop in KeyboardShortcut isn't implemented correctly.
When we click outside New Group page, all the eventListeners for
Escapekey are deleted.Proposal
Instead of
delete events[keyCode], doevents[keyCode].pop().https://github.com/Expensify/Expensify.cash/blob/b9569af14376b23fd8633883f2a3126a91d3fae4/src/libs/KeyboardShortcut/index.js#L129
Loop
eventCallbacksin reverse and return early when the right event is triggered.https://github.com/Expensify/Expensify.cash/blob/b9569af14376b23fd8633883f2a3126a91d3fae4/src/libs/KeyboardShortcut/index.js#L17
This is done because we don't want to trigger all the events of a key.
This'll fix escape key closing New Group, and Search at the same time, instead of one at a time.
Don't export the
unsubscribe()method, so that we're forced to take return value fromKeyboardShortcut.subscribe()Modify Authscreen.js to accept the the return value for unsubscribing.
Replace the callback of Escape key to
Navigation.closeDrawer()This changes the behaviour of Escape key from Go Back to Close.
Demo
solution.demo.mp4
parasharrajat commentedon Jul 14, 2021
This needs further planning.
rushatgabhane commentedon Jul 15, 2021
slack convo here
mallenexpensify commentedon Jul 16, 2021
Added
Dailylabel so @puneetlath will see it for triage.@parasharrajat , can you elaborate what needs further planning? I saw @rushatgabhane 's Slack post but there are no comments there. We can followup in a comment there and 'share with room' to try to get more eyes on, if needed
rushatgabhane commentedon Jul 17, 2021
I don't think further planning is required.
Just need feedback on my proposal in case I'm missing something.
parasharrajat commentedon Jul 18, 2021
So via further planning, I meant what has desired behaviour of ESC? If you look at the cross button on the Modal screen it does not take you back instead close the whole Modal Screen Stack and take you back to Report Screen. I think Esc is meant to do the same. Does Esc navigate back in the browser history? Open any page and navigate the website. Press ESC, will it go back in history?
rushatgabhane commentedon Jul 18, 2021
@parasharrajat I don't think so.
Because if you take a look at callbacks for Escape key,
goBackis set as the callback.https://github.com/Expensify/Expensify.cash/blob/fc965f68d308167d19d00a29b91e85ca33707882/src/components/ScreenWrapper.js#L61-L62
This tells me that the expected behaviour is to go back, instead of closing all the Modal screens.
mallenexpensify commentedon Jul 19, 2021
Posted to Upwork, assigning an engineer to review.
https://www.upwork.com/jobs/~011663eb89a2b43197
10 remaining items
puneetlath commentedon Jul 21, 2021
Great. I went ahead and hired @rushatgabhane in Upwork.
MelvinBot commentedon Jul 26, 2021
@tgolen, @puneetlath, @rushatgabhane Eep! 4 days overdue now. Issues have feelings too...
rushatgabhane commentedon Jul 26, 2021
Just waiting for review of PR 🙂
[-]KeyboadShortcut is broken[/-][+][Hold for payment Aug 3] KeyboadShortcut is broken[/+]MelvinBot commentedon Aug 2, 2021
@tgolen, @puneetlath, @rushatgabhane it looks like no one is assigned to work on this job.
Please double the price or add a comment stating why the job isn't being doubled.
mallenexpensify commentedon Aug 3, 2021
^ was from a test, disregard
puneetlath commentedon Aug 3, 2021
Paid!