[HOLD for payment 2023-03-06] [HOLD for payment 2023-02-10] [$1000] Timezone changes to some 1st option in the list while pressing the enter key #14314
Comments
kavimuru
added
Daily
Bug0 Triage ChecklistNote: see this SO for more information.
Reproducible, timezone selects the first option in the list when hitting enter, even when a different timezone is written in the text box: 2023-01-18_14-02-26.mp4 |
kadiealexander
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~01faba6d65b765cfed |
|
Current assignee @kadiealexander is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane ( |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to @sketchydroide ( |
ProposalThis happens due to we're focusing on the first element always upon rendering the OptionList, and when we press enter it will select the currently focused element. To fix this we need to focus on the currently selected element, if an element was selected prior to rendering the OptionList diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js
index c473c426d8..b1d5e01957 100755
--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -39,9 +39,17 @@ class BaseOptionsSelector extends Component {
this.relatedTarget = null;
const allOptions = this.flattenSections();
+
+ const selectedOptionIndex = _.findIndex(allOptions, option => option.text === this.props.value);
+
+ let focusedIndex = this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0;
+ if (selectedOptionIndex >= 0) {
+ focusedIndex = selectedOptionIndex;
+ }
+
this.state = {
allOptions,
- focusedIndex: this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0,
+ focusedIndex,
};
}
diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce7..7fc47ee5e8 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -61,6 +61,9 @@ class TimezoneSelectPage extends Component {
*/
saveSelectedTimezone({text}) {
PersonalDetails.updateSelectedTimezone(text);
+ this.setState({
+ timezoneInputText: text,
+ });
}
/**I'm using the We might need to fix this in other places as well with the same update |
Proposal 2If we don't want it to affect anything else, then can introduce a new params in the diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js
index c473c426d8..b8d32c0015 100755
--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -39,9 +39,18 @@ class BaseOptionsSelector extends Component {
this.relatedTarget = null;
const allOptions = this.flattenSections();
+
+ let focusedIndex = this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0;
+
+ const focusedOptionIndex = _.findIndex(allOptions, option => option.text === this.props.focusedValue);
+
+ if (focusedOptionIndex >= 0) {
+ focusedIndex = focusedOptionIndex;
+ }
+
this.state = {
allOptions,
- focusedIndex: this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0,
+ focusedIndex,
};
}
diff --git a/src/components/OptionsSelector/optionsSelectorPropTypes.js b/src/components/OptionsSelector/optionsSelectorPropTypes.js
index 306b89a26e..31150e7113 100644
--- a/src/components/OptionsSelector/optionsSelectorPropTypes.js
+++ b/src/components/OptionsSelector/optionsSelectorPropTypes.js
@@ -27,6 +27,9 @@ const propTypes = {
/** Value in the search input field */
value: PropTypes.string.isRequired,
+ /** Value of the option that we should focused on when first opening the options page */
+ focusedValue: PropTypes.string,
+
/** Callback fired when text changes */
onChangeText: PropTypes.func.isRequired,
diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce7..c392057ec0 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -61,6 +61,9 @@ class TimezoneSelectPage extends Component {
*/
saveSelectedTimezone({text}) {
PersonalDetails.updateSelectedTimezone(text);
+ this.setState({
+ timezoneInputText: text,
+ });
}
/**
@@ -90,6 +93,7 @@ class TimezoneSelectPage extends Component {
optionHoveredStyle={styles.hoveredComponentBG}
sections={[{data: this.state.timezoneOptions}]}
shouldHaveOptionSeparator
+ focusedValue={this.state.timezoneInputText}
/>
</ScreenWrapper>
);
|
|
Working well after both proposals: Screen.Recording.2023-01-18.at.09.21.50.mov |
ProposalIf we set the diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce7..f3d4a229c3 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -88,7 +88,7 @@ class TimezoneSelectPage extends Component {
onChangeText={this.filterShownTimezones}
onSelectRow={this.saveSelectedTimezone}
optionHoveredStyle={styles.hoveredComponentBG}
- sections={[{data: this.state.timezoneOptions}]}
+ sections={[{data: this.state.timezoneOptions, indexOffset: 0}]}
shouldHaveOptionSeparator
/>
</ScreenWrapper>
Result Screen.Recording.2023-01-17.at.18.52.19.mov |
If we want to avoid this minor issue, then we can default scroll to the selected element when we open the list (in diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js
index c473c426d8..60b53f294c 100755
--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -93,6 +103,8 @@ class BaseOptionsSelector extends Component {
} else {
this.textInput.focus();
}
+
+ this.scrollToIndex(this.state.focusedIndex);
}
I've tested the timezone selection on Slack and these are consistent with Slack's behavior:
Think of the user's viewpoint, if he changes the timezone it's likely will be surrounding timezone in |
|
There is another place that is also having this problem which may also need to be fixed Screen.Recording.2023-01-18.at.10.39.51.mov |
|
I think pressing enter should select the 1st option, that's the expected behaviour not a bug. The bug is that from the user perspective this is confusing because we are missing the focus state indication on the current focused row. I think all what we need to do here is apply @bernhardoj proposal here |
|
The bug is that the selection is not visible, but I will go a bit further. Example:
|
Proposaldiff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js
index c473c426d8..904de0450d 100755
--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -41,7 +41,7 @@ class BaseOptionsSelector extends Component {
const allOptions = this.flattenSections();
this.state = {
allOptions,
- focusedIndex: this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0,
+ focusedIndex: props.shouldTextInputAppearBelowOptions ? allOptions.length : 0,
};
}
@@ -84,6 +84,10 @@ class BaseOptionsSelector extends Component {
true,
);
+ if (this.props.focusedItemKey) {
+ this.updateFocusedIndex(_.findIndex(this.state.allOptions, option => option.keyForList === this.props.focusedItemKey), false);
+ }
+
if (!this.props.autoFocus) {
return;
}
@@ -166,17 +170,19 @@ class BaseOptionsSelector extends Component {
/**
* @param {Number} index
+ * @param {Boolean} animated
*/
- updateFocusedIndex(index) {
- this.setState({focusedIndex: index}, () => this.scrollToIndex(index));
+ updateFocusedIndex(index, animated = true) {
+ this.setState({focusedIndex: index}, () => this.scrollToIndex(index, animated));
}
/**
* Scrolls to the focused index within the SectionList
*
* @param {Number} index
+ * @param {Boolean} animated
*/
- scrollToIndex(index) {
+ scrollToIndex(index, animated = true) {
const option = this.state.allOptions[index];
if (!this.list || !option) {
return;
@@ -195,7 +201,7 @@ class BaseOptionsSelector extends Component {
}
}
- this.list.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex});
+ this.list.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated});
}
/**
diff --git a/src/components/OptionsSelector/optionsSelectorPropTypes.js b/src/components/OptionsSelector/optionsSelectorPropTypes.js
index 306b89a26e..51691fb41e 100644
--- a/src/components/OptionsSelector/optionsSelectorPropTypes.js
+++ b/src/components/OptionsSelector/optionsSelectorPropTypes.js
@@ -89,6 +89,9 @@ const propTypes = {
/** Whether to show a line separating options in list */
shouldHaveOptionSeparator: PropTypes.bool,
+
+ /** Initially focused item key */
+ focusedItemKey: PropTypes.string,
};
const defaultProps = {
@@ -113,6 +116,7 @@ const defaultProps = {
disableArrowKeysActions: false,
isDisabled: false,
shouldHaveOptionSeparator: false,
+ focusedItemKey: '',
};
export {propTypes, defaultProps};
diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce7..2018ca275a 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -88,7 +88,8 @@ class TimezoneSelectPage extends Component {
onChangeText={this.filterShownTimezones}
onSelectRow={this.saveSelectedTimezone}
optionHoveredStyle={styles.hoveredComponentBG}
- sections={[{data: this.state.timezoneOptions}]}
+ sections={[{data: this.state.timezoneOptions, indexOffset: 0}]}
+ focusedItemKey={this.currentSelectedTimezone}
shouldHaveOptionSeparator
/>
</ScreenWrapper>RCAThis seems more like a feature request: scroll to selected item
Note: I have only applied this on the timezone page, looks like we can't apply this on the currency selector in an easy way since we don't have a direct access the current selected currency. ResultsKooha-2023-01-19-18-47-50.mp4 |
|
@s77rt looks like your proposal has rather similar approach to my proposals in here and here (the addition of ‘animated: false’ was also covered). The use of ‘keyForList’ was also covered here already. Regarding the missing focus, I think that might be a conscious design decision since we already have the green check mark to indicate the selected option. You can check the Pronouns selection, it also follows the same behavior. If someone in the team agrees to fix this missing focus, I’d propose to fix in both Timezones and Pronouns as well by adding the ’indexOffset: 0’. diff --git a/src/pages/settings/Profile/PronounsPage.js b/src/pages/settings/Profile/PronounsPage.js
index 8665187789..5be60769a3 100644
--- a/src/pages/settings/Profile/PronounsPage.js
+++ b/src/pages/settings/Profile/PronounsPage.js
@@ -63,7 +63,7 @@ const PronounsPage = (props) => {
{props.translate('pronounsPage.isShownOnProfile')}
</Text>
<OptionsList
- sections={[{data: pronounsList}]}
+ sections={[{data: pronounsList, indexOffset: 0}]}
onSelectRow={option => updatePronouns(option.value)}
hideSectionHeaders
optionHoveredStyle={styles.hoveredComponentBG}
diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index c392057ec0..6ccffdcd85 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -91,7 +91,7 @@ class TimezoneSelectPage extends Component {
onChangeText={this.filterShownTimezones}
onSelectRow={this.saveSelectedTimezone}
optionHoveredStyle={styles.hoveredComponentBG}
- sections={[{data: this.state.timezoneOptions}]}
+ sections={[{data: this.state.timezoneOptions, indexOffset: 0}]}
shouldHaveOptionSeparator
focusedValue={this.state.timezoneInputText}
/>
The correct behavior agreed here is also missing in Pronouns and will need to be fixed similar to this. |
|
@tienifr The proposal may similar but not the same, you didn't provide a complete proposal. We do things a bit different e.g. the way we update the focused index. |
|
@tienifr your proposal does not scroll the list to current set timezone (at least I did not see it on the video)
I think that might be wrong as well, but I might need to ask design, I think it's good UX practice to show the current selected value @s77rt your seems to work on the video @Expensify/design What do you thing the behaviour should be here? |
|
@sketchydroide the scroll part is in the second part of my proposal here. The video you see was the part before that so it didn't have the scroll. Sorry for the inconvenience Let me paste the full diff for my existing proposals above for ease of review: diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js
index c473c426d8..a5a4337d83 100755
--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -39,9 +39,18 @@ class BaseOptionsSelector extends Component {
this.relatedTarget = null;
const allOptions = this.flattenSections();
+
+ let focusedIndex = this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0;
+
+ const focusedOptionIndex = _.findIndex(allOptions, option => option.text === this.props.focusedValue);
+
+ if (focusedOptionIndex >= 0) {
+ focusedIndex = focusedOptionIndex;
+ }
+
this.state = {
allOptions,
- focusedIndex: this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0,
+ focusedIndex,
};
}
@@ -93,6 +102,8 @@ class BaseOptionsSelector extends Component {
} else {
this.textInput.focus();
}
+
+ this.scrollToIndex(this.state.focusedIndex, false);
}
componentDidUpdate(prevProps) {
@@ -175,8 +186,9 @@ class BaseOptionsSelector extends Component {
* Scrolls to the focused index within the SectionList
*
* @param {Number} index
+ * @param {Boolean} animated
*/
- scrollToIndex(index) {
+ scrollToIndex(index, animated = true) {
const option = this.state.allOptions[index];
if (!this.list || !option) {
return;
@@ -195,7 +207,7 @@ class BaseOptionsSelector extends Component {
}
}
- this.list.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex});
+ this.list.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated});
}
/**
diff --git a/src/components/OptionsSelector/optionsSelectorPropTypes.js b/src/components/OptionsSelector/optionsSelectorPropTypes.js
index 306b89a26e..31150e7113 100644
--- a/src/components/OptionsSelector/optionsSelectorPropTypes.js
+++ b/src/components/OptionsSelector/optionsSelectorPropTypes.js
@@ -27,6 +27,9 @@ const propTypes = {
/** Value in the search input field */
value: PropTypes.string.isRequired,
+ /** Value of the option that we should focused on when first opening the options page */
+ focusedValue: PropTypes.string,
+
/** Callback fired when text changes */
onChangeText: PropTypes.func.isRequired,
diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce7..6ccffdcd85 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -61,6 +61,9 @@ class TimezoneSelectPage extends Component {
*/
saveSelectedTimezone({text}) {
PersonalDetails.updateSelectedTimezone(text);
+ this.setState({
+ timezoneInputText: text,
+ });
}
/**
@@ -88,8 +91,9 @@ class TimezoneSelectPage extends Component {
onChangeText={this.filterShownTimezones}
onSelectRow={this.saveSelectedTimezone}
optionHoveredStyle={styles.hoveredComponentBG}
- sections={[{data: this.state.timezoneOptions}]}
+ sections={[{data: this.state.timezoneOptions, indexOffset: 0}]}
shouldHaveOptionSeparator
+ focusedValue={this.state.timezoneInputText}
/>
</ScreenWrapper>
);
As mentioned in here we might consider using other sorts of id (like keyForList), but that's an implementation detail. After the fixScreen.Recording.2023-01-20.at.18.20.10.mov |
|
@rushatgabhane what do you think? |
|
I like the idea of showing the selected item in the center of the view, aka scrolling to the right position when navigating back to the view with these selection lists (pronoun, currency, timezone, etc). However, I don't like how in this proposal here that the view seems to be moving a bit as it comes into view. cc @Beamanator too since I think you worked on implementing this? |
|
@sketchydroide, @rushatgabhane, @kadiealexander, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick! |
|
@thienlnam do you want to take over this issue? |
|
@Beamanator The PR is created again and it is being reviewed by @rushatgabhane |
|
@tienifr thanks, looks like you're talking about #14938? I think @thienlnam or I can take over from the expensify side |
@Beamanator yes I think we can just speed up reviewing that PR, it has the same changes as the one approved and merged previously. It didn't cause the regression but was reverted by mistake. The regression tests for the related issues were already added as well. |
|
@Beamanator Yeah, I can take over this issue |
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.76-7 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 2023-03-06. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External 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:
|
|
I created regression test before
|
|
@priya-zha, @tienifr and @rushatgabhane please apply here so we can get you paid :) |
|
@kadiealexander Upwork doesn't allow me to apply for this job. I get an error even though I'm logged in with the right account.
|
|
📣 @priya-zha! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Format: |
|
@priya-zha could you please complete the steps above first? |
|
Contributor details |
|
Thanks @priya-zha! I have sent you a contract in Upwork, please accept and we'll issue payment. |
|
@kadiealexander Thank you. I've accepted the offer. |
|
Awesome, all paid! Payment issued to @tienifr too. @thienlnam are you happy with the regression test proposed here? |
|
Sorry for the delay on this - could we please try to be more specific in the regression tests so it's easier for our QA team to follow? @tienifr Things like updating this
to: Also, we want to test the success case so :
We don't want this to happen right? So
|
|
@thienlnam Thanks for your comments Regression Test ProposalBug: Timezone changes to some 1st option in the list while pressing the enter key Proposed Test Steps:
Video (Regression Test): Screen.Recording.2023-02-13.at.15.48.56.mp4 |
|
Awesome, looks great now - thanks! |
|
#15940 - GH to get regression test added to TestFlight. @thienlnam is there anything else pending before we close this out? |
|
All good here, thanks @kadiealexander ! |
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:
When showing the timezone list the timezone that was set should be selected and visible and pressing the enter key should select the current selected timezone (in case no interaction from the user was made then nothing should change)
Actual Result:
When opening the timezone selection the actual set timezone does not apperar selected, and when pressing enter the first item on the list is seleced instead.
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.54-2
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:
timezone.mp4
Recording.1293.mp4
Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673705115960529
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: