Closed kbecciv closed 12 months ago
Triggered auto assignment to @Christinadobrzyn (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are β
)Job added to Upwork: https://www.upwork.com/jobs/~01f0d617af5beeb293
Triggered auto assignment to @joekaufmanexpensify (External
), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External
)
@Christinadobrzyn I was duplicate assigned here, so un-assigning as this doesn't need two BZ members. We are fixing this here.
Member highlights on deselect all members in workspace members page.
If the user select/deselect a member, selectRow
function is called and the function focuses the next member in the list.
This is implemented for continuous selection & deselection by Enter
keyboard shortcut.
The code is written inside selectRow
function and it will be called from selectFocusedOption
.
But selectRow
function is also called if the user select/deselect by click/press.
So the next member will be focused too and we see current issue.
I think we need to call onSelectRow
function directly instead of calling selectRow
function if the user select/deselect by click/press.
So the app will only toggle the item without moving focus to the next item.
N/A
The logic "Auto focus on the next available item" is only useful when the user uses the ENTER key to select/deselect the items. So we can add the condition to appy this auto-focus logic: The device are desktop/desktop-web. Like below:
const selectRow = (item, index) => {
// In single-selection lists we don't care about updating the focused index, because the list is closed after selecting an item
if (canSelectMultiple && !DeviceCapabilities.canUseTouchScreen()) {
// Auto-focus logic
}
onSelectRow(item);
};
- Based on ```DeviceCapabilities.canUseTouchScreen()```, we maybe can check if the device is desktop or not
### What alternative solutions did you explore? (Optional)
- Also we can just apply the auto-focus next item in case user press the ENTER key to select the items like:
const selectRow = (item, index, isPressingEnter) => { if(isPressingEnterKey) { // Logic auto-focus here} onSelectRow(item) }
- The isPressingEnter `s default value is false. So the clicking item does not auto-focus on next available item. In the callback function to handle ENTER press - ```selectFocusedOption```, we will call selectRow with isPressingEnter is true
### Result
- First proposal: Mobile web
[Screencast from 14-09-2023 12:25:47.webm](https://github.com/Expensify/App/assets/141406735/7fd32001-a761-44c9-a6e3-7e23215f544a)
@akamefi202 Thanks for the proposal. Your RCA is correct and the solution looks good to me.
π π π C+ reviewed Link to proposal
Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@DylanDylann Thanks for the proposal. Your RCA is correct. The first solution i.e. the use of DeviceCapabilities.canUseTouchScreen
does not seem correct for this case (the feature is related to keyboard not touchscreen) and the second solution is a pretty much the same as the one proposed by @akamefi202.
@s77rt I think based on DeviceCapabilities.canUseTouchScreen and maybe any additional conditions in the detail PR, we can know that whether the device is desktop or not. So only desktop device can use the keyboard in this case, right? The main idea from my proposal is that: We will disable the auto-focus logic with the devices that do not have ENTER button
Hey @techievivek can you take a peek at this https://github.com/Expensify/App/issues/27377#issuecomment-1719886245
@DylanDylann The !DeviceCapabilities.canUseTouchScreen()
condition would be true on Web/Desktop, this won't change anything and the issue would be still reproducible on those platforms.
@s77rt why "this won't change anything and the issue would be still reproducible on those platforms."? In mobile, !DeviceCapabilities.canUseTouchScreen() will be false so the auto-focus logic is not applied based on my proposal. Please help give me more detail
Also, should we need to confirm whether the auto-focus on the item when deselecting item is intention or not @s77rt
@DylanDylann
why "this won't change anything and the issue would be still reproducible on those platforms."? In mobile, !DeviceCapabilities.canUseTouchScreen() will be false so the auto-focus logic is not applied
Right, but on Web/Desktop the issue would be still there. We need to fix the bug for all the platforms.
should we need to confirm whether the auto-focus on the item when deselecting item is intention or not
It's intentional based on the code comments
π£ @s77rt π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @akamefi202 π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
π£ @gadhiyamanan π An offer has been automatically sent to your Upwork account for the Reporter role π Thanks for contributing to the Expensify app!
Thank you. The PR will be ready by tomorrow.
@s77rt I closed the PR.
Reached out to BZ about payment - https://expensify.slack.com/archives/C01SKUP7QR0/p1695743819720659
Follow up from the team is to do a case-by-case evaluation of the work done on the GH and PR.
@s77rt @akamefi202 @techievivek what do you think is fair payment for the work on this job/PR?
I think 50% would be fair for this case.
Sounds good! Sorry, I'm a little lost here... The job was fixed elsewhere and we can close this, right?
How's this for payouts due:
Issue Reporter: $50 @gadhiyamanan Contributor: $250 @akamefi202 Contributor+: $250 @s77rt
Eligible for 50% #urgency bonus? N
Upwork job is here.
@s77rt @Christinadobrzyn @techievivek Can we please close this issue soon if everything is clear here?
@Christinadobrzyn Sorry I missed the question
The job was fixed elsewhere and we can close this, right?
Yes. it was fixed here https://github.com/Expensify/App/pull/27246
Thanks! Paid this out in Upwork based on https://github.com/Expensify/App/issues/27377#issuecomment-1741549948
Closing!
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:
Expected Result:
Member should not be highlights
Actual Result:
Member highlights
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.69.0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation
https://github.com/Expensify/App/assets/93399543/75dc0bd5-e066-40c2-b3fc-5bdf71e0774b
https://github.com/Expensify/App/assets/93399543/c5ed47cf-13a6-4eb6-ae8d-ec7a49295e10
Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694422514518809
View all open jobs on GitHub
Upwork Automation - Do Not Edit