Skip to content
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

fix: Timezone changes to some 1st option in the list while pressing the enter key #14466

Merged
merged 12 commits into from
Feb 1, 2023
Merged
9 changes: 6 additions & 3 deletions src/components/OptionsList/BaseOptionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ class BaseOptionsList extends Component {
* @returns {Array<Object>}
*/
buildFlatSectionArray() {
const optionHeight = variables.optionRowHeight;
let offset = 0;

// Start with just an empty list header
Expand All @@ -121,8 +120,12 @@ class BaseOptionsList extends Component {

// Add section items
for (let i = 0; i < section.data.length; i++) {
flatArray.push({length: optionHeight, offset});
offset += optionHeight;
let fullOptionHeight = variables.optionRowHeight;
if (i > 0 && this.props.shouldHaveOptionSeparator) {
fullOptionHeight += variables.borderTopWidth;
}
flatArray.push({length: fullOptionHeight, offset});
offset += fullOptionHeight;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tienifr

suggestion: i think this is slightly easier to read

buildFlatSectionArray()

  for () 
      let fullOptionHeight = variables.optionRowHeight
      offset += variables.optionRowHeight
      if (i > 0 && shouldHaveOptionsSeperator)
          offset += variables.borderTopWidth 
          fullOptionHeight += variables.borderTopWidth
   
     flatArray.push({length: fullOptionHeight, offset})

      

Copy link
Contributor Author

@tienifr tienifr Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane I think the above might be incorrect since the offset that's pushed to flatArray is supposed to be for this item. Meaning we should not add this item's height to the offset before pushing to the flatArray (the offset should be the sum of all the heights of the items before this item). That's why the offset calculation happens after the flatArray.push in the current code, the next item will use this calculated offset, not this item

flatArray.push({length: fullOptionHeight, offset});
offset += fullOptionHeight;

This comment was marked as outdated.

Copy link
Member

@rushatgabhane rushatgabhane Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes you're right.

offset is the height till the top of the current option. we don't include the current option itself

}

// Add the section footer
Expand Down
38 changes: 34 additions & 4 deletions src/components/OptionsSelector/BaseOptionsSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,11 @@ class BaseOptionsSelector extends Component {
this.relatedTarget = null;

const allOptions = this.flattenSections();
const focusedIndex = this.getInitiallyFocusedIndex(allOptions);

this.state = {
allOptions,
focusedIndex: this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0,
focusedIndex,
};
}

Expand Down Expand Up @@ -84,6 +86,8 @@ class BaseOptionsSelector extends Component {
true,
);

this.scrollToIndex(this.state.focusedIndex, false);

if (!this.props.autoFocus) {
return;
}
Expand Down Expand Up @@ -135,6 +139,25 @@ class BaseOptionsSelector extends Component {
}
}

/**
* @param {Array<Object>} allOptions
* @returns {Number}
*/
getInitiallyFocusedIndex(allOptions) {
const defaultIndex = this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0;
if (_.isUndefined(this.props.initiallyFocusedOptionKey)) {
return defaultIndex;
}

const indexOfInitiallyFocusedOption = _.findIndex(allOptions, option => option.keyForList === this.props.initiallyFocusedOptionKey);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tienifr OptionSelector is a heavy component.

enhancement: we should skip this O(n) calculation when props.initiallyFocusedOptionKey is undefined.

const defaultIndex = this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0;

if (_.isUndefined(props.initiallyFocusedOptionKey)) {
    return defaultIndex;
}

const intiallyFocusedOptionIndex = _.findIndex(allOptions, option => option.keyForList === this.props.initiallyFocusedOptionKey);

if (intiallyFocusedOptionIndex === -1) {
  return defaultIndex;
}

return intiallyFocusedOptionIndex;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane updated according to your suggestion, with the minor variation where the return defaultIndex; is the last return rather than return intiallyFocusedOptionIndex;. Since we might add other logic to calculate the focused index in the future, I feel that the defaultIndex should be the fallback return. Let me know if you feel strongly that intiallyFocusedOptionIndex should be the last return instead.

Thanks

         if (indexOfInitiallyFocusedOption >= 0) {
             return indexOfInitiallyFocusedOption;
         }

         return defaultIndex;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's good reasoning. looks good!


if (indexOfInitiallyFocusedOption >= 0) {
return indexOfInitiallyFocusedOption;
}

return defaultIndex;
}

/**
* Flattens the sections into a single array of options.
* Each object in this array is enhanced to have:
Expand Down Expand Up @@ -175,8 +198,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;
Expand All @@ -195,7 +219,7 @@ class BaseOptionsSelector extends Component {
}
}

this.list.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex});
this.list.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the changes we would be animating all the scrollToLocation calls

question: was it always like this before? does this.list.scrollToLocation animate by default?

Copy link
Contributor Author

@tienifr tienifr Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane yes, scrollToLocation has animated: true by default if we do not pass any animated params in.

Reference: https://reactnative.dev/docs/sectionlist, the scrollToLocation section

}

/**
Expand Down Expand Up @@ -265,7 +289,13 @@ class BaseOptionsSelector extends Component {
showTitleTooltip={this.props.showTitleTooltip}
isDisabled={this.props.isDisabled}
shouldHaveOptionSeparator={this.props.shouldHaveOptionSeparator}
onLayout={this.props.onLayout}
onLayout={() => {
this.scrollToIndex(this.state.focusedIndex, false);

if (this.props.onLayout) {
this.props.onLayout();
Copy link
Member

@rushatgabhane rushatgabhane Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tienifr there's a "flash" / flicker when you click on timezone

Make sure to test on a physical android device.
I verified that it isn't present on main.

I think it has something to do with scrollToIndex and keyboard opening at the same time.

WhatsApp.Video.2023-01-31.at.01.23.23.mp4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's main for reference. There is no flicker

WhatsApp.Video.2023-01-31.at.05.12.38.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane pushed the fix. It's working fine as below:

Screenrecorder-2023-01-31-10-23-50-964.mp4

The reason was because of the scrollToIndex is running inside onLayout, for Android the list will appear for a moment before onLayout is called, thus causing the flicker.

The update was to run the scrollToIndex on both componentDidMount and onLayout. We still need the scrollToIndex call in onLayout because on iOS, the scrollToLocation API does not work if the layout was not calculated yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane since it's close to passing the 6-days milestone since the issue was assigned, I'd appreciate if we can coordinate to move this quickly.

Thanks!

Copy link
Member

@rushatgabhane rushatgabhane Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, left a review #14466 (review)

i think scrollToLocation in component update is causing a regression

Copy link
Member

@rushatgabhane rushatgabhane Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind ^, i think it was some random issue bug because i was doing something wrong.

}
}}
/>
) : <FullScreenLoadingIndicator />;
return (
Expand Down
4 changes: 4 additions & 0 deletions src/components/OptionsSelector/optionsSelectorPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ const propTypes = {

/** Whether to show a line separating options in list */
shouldHaveOptionSeparator: PropTypes.bool,

/** Key of the option that we should focus on when first opening the options list */
initiallyFocusedOptionKey: PropTypes.string,
};

const defaultProps = {
Expand All @@ -113,6 +116,7 @@ const defaultProps = {
disableArrowKeysActions: false,
isDisabled: false,
shouldHaveOptionSeparator: false,
initiallyFocusedOptionKey: undefined,
};

export {propTypes, defaultProps};
3 changes: 2 additions & 1 deletion src/pages/settings/Profile/TimezoneSelectPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,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}]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tienifr hey, I don't understand this change. Could you please help me out here 🙇‍♂️

why do we need to pass indexOffset as 0 ?

Copy link
Contributor Author

@tienifr tienifr Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane it’s for showing the focus style (background color changing) for the focused option. At timezone selection page, we forget to pass the indexOffset to the sections making the option item focus style didn't show.
You can see the indexOffset: 0 being used for the OptionsSelector in other pages like IOUCurrencySelection, ReportParticipantsPage, ...

shouldHaveOptionSeparator
initiallyFocusedOptionKey={this.currentSelectedTimezone}
/>
</ScreenWrapper>
);
Expand Down
2 changes: 1 addition & 1 deletion src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -1788,7 +1788,7 @@ const styles = {
},

borderTop: {
borderTopWidth: 1,
borderTopWidth: variables.borderTopWidth,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

borderColor: themeColors.border,
},

Expand Down
1 change: 1 addition & 0 deletions src/styles/variables.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,5 @@ export default {
checkboxLabelActiveOpacity: 0.7,
avatarChatSpacing: 12,
chatInputSpacing: 52, // 40 + avatarChatSpacing
borderTopWidth: 1,
};