-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2024-02-26] [$500] App does not focus to first option on search if selected from first page option in timezone #29080
Comments
Job added to Upwork: https://www.upwork.com/jobs/~0178208b9c15b89907 |
Triggered auto assignment to @jliexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?This issue occurs because we dont change the focus to the first item when the What changes do you think we should make in order to solve the problem?To change the focus on the first item whenever the useEffect(() => {
//do not change focus on the first render, as it should focus on the selected item
if (firstLayoutRef.current) {
return;
}
// set the focus on the first item when the sections list is changed
if (sections.length > 0) {
updateAndScrollToFocusedIndex(0);
}
}, [sections]); Alternative Solution:Change so we should add this <OptionsSelector
headerMessage={
timezoneInputText.trim() && !timezoneOptions.length
? translate("common.noResultsFound")
: ""
}
textInputLabel={translate("timezonePage.timezone")}
value={timezoneInputText}
onChangeText={filterShownTimezones}
onSelectRow={saveSelectedTimezone}
sections={[
{ data: timezoneOptions, indexOffset: 0, isDisabled: timezone.automatic },
]}
initiallyFocusedOptionKey={_.get(
_.filter(timezoneOptions, (tz) => tz.text === timezone.selected)[0],
"keyForList"
)}
showScrollIndicator
/> and also import the const greenCheckmark = {
src: Expensicons.Checkmark,
color: themeColors.success,
}; and use it in the const allTimezones = useRef(
_.chain(TIMEZONES)
.filter((tz) => !tz.startsWith("Etc/GMT"))
.map((text) => ({
text,
customIcon: text === timezone.selected ? greenCheckmark : undefined,
keyForList: getKey(text),
isSelected: text === timezone.selected,
}))
.value()
); POC:Screen.Recording.2023-10-08.at.2.31.41.AM.movScreen.Recording.2023-10-20.at.2.10.21.AM.mov |
@jliexpensify Can you reassign a new C+ by applying |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @robertKozik ( |
Not able to replicate |
Thanks @maxconnectAbhi - looks like the app is v80-1, so I will also confirm |
Hmm it still seems to be an issue on the Android app - when Is search |
"@jliexpensify, is the focus applied on the first one option though? Correct me if I'm wrong, but from your comment it looks like a different issue connected with the search function." |
Hmm maybe? I think this is what I'm unclear on:
Does this mean that when typing |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
I came back to it, and I can confirm that this bug is still reproductible. I'll review the proposal |
@abzokhattab, I appreciate your proposal. Since you already have the proof of concept implemented, could you verify if it behaves as intended for the case shown in the video? Specifically, select Africa/Asmara and then type in. I've found it to be quite dependent on user input, so I'd appreciate a double-check on a scenario that is currently not working. |
Hi @robertKozik yeah i already used let me know if you want to test it with any other values |
Thanks @abzokhattab for checking. Let's see your proposal in action 🪨 🎀 👀 🎀 C+ reviewed |
Yes the new PR will fix the issue you are referring to @jjcoffee as this condition in the However, the focus appears only when you start typing otherwise no element is focused. if we want to set the selection on the first unselected element when selecting a row as well then we should
onSelectRow={() => selectRow(item)} FYI the what do you think @fedirjh should we remove this argument? |
The bug seems to be fixed in the current version of the PR, so let's avoid any unnecessary changes. |
This issue has not been updated in over 15 days. @jliexpensify, @MonilBhavsar, @fedirjh, @abzokhattab eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.42-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-02-26. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
BugZero Checklist:
Regression Test Proposal
|
Wow this is an old issue, and also a regression? @MonilBhavsar can you check this summary?
|
I have two points here
Please let me know |
Hi @abzokhattab - thanks for surfacing this! Just clarifying: were you paid for the other job? If so, the 50% regression penalty should have been applied there. My understanding is that if a regression occurs, then the original Contributor is assigned to the new bug to also resolve that. I had always thought that this counted as part of the Contributor "resolving" the regression, but perhaps my understanding is incorrect? I'll review the .MD. |
Hi @abzokhattab - just circling back: the .MD isn't super clear (it just has one line around regression payments), but I confirmed with a colleague:
For this one, you'd see a 50% deduction on your original job. @fedirjh is paid the full amount as he's a new C+ assigned to the PR. |
Thanks @jliexpensify but there was a second proposal assignment here, the issue was not auto-assigned to me again after the regression, however, fedirih and Monil had a long conversation discussing the proposals and agreed on my proposal which I think requires another payment. From my understanding, if my proposal wasn't chosen the second time, the compensation should be limited to $250. but since we had a second proposals assignment I think this is considered as a new issue. my proposals were selected here and here Thanks again for your understanding. please let me know what you think |
Ok, so I reviewed the entire set of comments - as I am not an Engineer, I may not be understanding fully, but here's what I think:
A couple of clarifying questions from me:
|
Not to disturb the communication but just to inform, thanks @jliexpensify, I have accepted the offer on upwork. |
@jliexpensify @abzokhattab also needs to be paid $500, as this issue was fixed by two PR's. One PR that fixed original bug and another PR that fixed the regression. |
Got it, thanks @MonilBhavsar and apologies @abzokhattab - having that context really helps. My assumption was that this was a bug that was a regression, and every proposal after was focused on the regression. I've adjusted it to pay @abzokhattab $500 |
@MonilBhavsar just checking - you don't mean $500 + $250 (regression penalty) right? |
No, since they made a follow up to fix the issue completely. We are fine to pay $500 |
Paid and job closed! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
App should shift focus to first option on search in timezone
Actual Result:
After scroll to top and selecting any one timezone from visible options, app does not shift focus to first option on search in timezone
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.79.3
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
android.native.timezone.position.error.mov
Recording.4895.mp4
mac.chrome.desktop.timezone.maintains.search.position.1.mov
ios.safari.native.timezone.maintains.search.position.1.mov
windows.chrome.timezone.focus.not.shift.to.first.1.mp4
android.chrome.timezone.maintains.search.position.1.mp4
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696672477964269
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: