You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks @rushatgabhane , checking in on the Slack thread to see if we want to address other places we might want to 'fix' this at the same time. Do you know of any others? I can't think of any and I def noticed this one previously
Great job @rushatgabhane , that's a clean and clear OP. Can you submit a proposal for how you plan to fix the issue/s then @muttmuure will assign an engineer to review? (I'm OOO all next week.... ) Thanks again!
Convert the functional component Button to a class component.
Create a new prop pressOnEnter for Button.
If it's true, onPress() is called when Enter key is pressed.
This is done by adding Enter as a keyboard shortcut for onPress() using libs/KeyboardShortcut
componentDidMount() {
return if pressOnEnter is false
KeyboardShortcut.subscribe('Enter', () => {
if (button not disabled etc)
props.onPress()
})
}
Unsubscribe on unmount.
For all the pages like request payment, add workspace etc.
Pass the prop pressOnEnter to their buttons, so that onPress() is called when Enter key is pressed.
With this proposal, we can simply pass the prop to make any button be pressed with Enter key.
This'll cleanup IOUAmountPage too.
I don't think some of the extra code there was really necessary. As far as the naming of the prop, I actually liked pressOnEnter. I don't think bindEnter makes as much intuitive sense (what is being bound to enter and what consequence does that have?). I don't think we need to worry yet about binding any other keys either. Especially not for triggering onPress().
Thanks @parasharrajat , didn't scroll up far enough to see that. It looks like @tgolen wasn't assigned to review the PR because he unassigned himself and our bot/logic wasn't able to find him.
Paid in Upwork and added a $250 bonus (per process update last week) for posting the issue.
One thing before closing....
I found another example of where I think it'd be good to allow for usage of the enter key.
Add additional users to group text
Go to a group chat (command+shift+k is a shortcut)
Type user A name then hit enter when their name is at top.
Type user B name then hit enter when their name is at top.
Observe that User B isn't added to the group chat but replaces User A.
Expected result: User B is added along with User A, and any other additional users.
Bonus - once all users are added, allow to start chat by clicking shift+enter.
Reckon it's likely best if I create a new GH to have this addressed. Maybe create and put a HOLD on it to see if there are other examples we want to add too? @rushatgabhane Any thoughts on the above before I close out this issue?
Activity
MelvinBot commentedon Jun 29, 2021
Triggered auto assignment to @muttmuure (
AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.rushatgabhane commentedon Jun 29, 2021
mallenexpensify commentedon Jun 29, 2021
Thanks @rushatgabhane , checking in on the Slack thread to see if we want to address other places we might want to 'fix' this at the same time. Do you know of any others? I can't think of any and I def noticed this one previously
mallenexpensify commentedon Jun 29, 2021
From thread.... @parasharrajat recommends:
Yeah, a audit is good. There are few places to look for.
rushatgabhane commentedon Jun 30, 2021
@parasharrajat Thanks!
I've done an audit, and updated the issue to reflect the places this is happening.
@mallenexpensify should I proceed with a PR?
[-]Confirm payment request on Enter key press.[/-][+]Web/Desktop Press Enter key to go next.[/+]mallenexpensify commentedon Jul 2, 2021
Great job @rushatgabhane , that's a clean and clear OP. Can you submit a proposal for how you plan to fix the issue/s then @muttmuure will assign an engineer to review? (I'm OOO all next week.... ) Thanks again!
rushatgabhane commentedon Jul 6, 2021
Awesome!
@muttmuure here's my proposal 😃
Proposal
Convert the functional component
Buttonto a class component.Create a new prop
pressOnEnterforButton.If it's true,
onPress()is called whenEnterkey is pressed.This is done by adding
Enteras a keyboard shortcut foronPress()usinglibs/KeyboardShortcutUnsubscribe on unmount.
For all the pages like request payment, add workspace etc.
Pass the prop
pressOnEnterto their buttons, so thatonPress()is called whenEnterkey is pressed.With this proposal, we can simply pass the prop to make any button be pressed with Enter key.
This'll cleanup IOUAmountPage too.
MelvinBot commentedon Jul 12, 2021
Triggered auto assignment to @tgolen (
Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.5 remaining items
tgolen commentedon Jul 12, 2021
I think the proposal is mostly fine. I would simplify it more to just this:
I don't think some of the extra code there was really necessary. As far as the naming of the prop, I actually liked
pressOnEnter. I don't thinkbindEntermakes as much intuitive sense (what is being bound to enter and what consequence does that have?). I don't think we need to worry yet about binding any other keys either. Especially not for triggeringonPress().tgolen commentedon Jul 12, 2021
🟢 to move forward @rushatgabhane
mallenexpensify commentedon Jul 13, 2021
Hired in Upwork!
mallenexpensify commentedon Jul 18, 2021
@tgolen reassigning to you, you'll be the one reviewing the incoming PR (assuming this is working, which it should be)
parasharrajat commentedon Jul 18, 2021
@mallenexpensify It seems PR is already merged and created before proposal Approval.
mallenexpensify commentedon Jul 18, 2021
Thanks @parasharrajat , didn't scroll up far enough to see that. It looks like @tgolen wasn't assigned to review the PR because he unassigned himself and our bot/logic wasn't able to find him.
rushatgabhane commentedon Jul 19, 2021
@mallenexpensify I had created a draft PR, and published it when @tgolen approved the proposal.
Apologies for the confusion.
rushatgabhane commentedon Aug 3, 2021
@mallenexpensify Issue can be closed. Merged ~2 weeks ago.
mallenexpensify commentedon Aug 3, 2021
Paid in Upwork and added a $250 bonus (per process update last week) for posting the issue.
One thing before closing....
I found another example of where I think it'd be good to allow for usage of the enter key.
Add additional users to group text
Go to a group chat (command+shift+k is a shortcut)
Type user A name then hit
enterwhen their name is at top.Type user B name then hit
enterwhen their name is at top.Observe that User B isn't added to the group chat but replaces User A.
Expected result: User B is added along with User A, and any other additional users.
Bonus - once all users are added, allow to start chat by clicking
shift+enter.Reckon it's likely best if I create a new GH to have this addressed. Maybe create and put a HOLD on it to see if there are other examples we want to add too? @rushatgabhane Any thoughts on the above before I close out this issue?
rushatgabhane commentedon Aug 3, 2021
Thanks!
@mallenexpensify I would've fixed it in this issue itself, but
Enterworks as intended.The thing that'll need to be changed is how search works for New Group and that'll need some planning.
mallenexpensify commentedon Aug 3, 2021
Dropped in #expensify-open-source as a feature request https://expensify.slack.com/archives/C01GTK53T8Q/p1628002164119000