Fix/16184 base options selector refactoring #31606
Conversation
melvin-bot
bot
requested review from
thienlnam
and removed request for
a team
|
@thienlnam Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Hey! Can you please make link the associated issue in the PR description next time? That's required for our automation to auto-assign the correct people to review the PR - thanks! |
|
Also, looks like we have some merge conflicts |
| const enterSubscription = useRef(); | ||
| const CTRLEnterSubscription = useRef(); |
There was a problem hiding this comment.
I don't think we need this. Just define it inside useEffect only.
|
Update: today im on sick leave, have everything almost finished, will provide proposal with fixes tomorrow |
|
Finishing testings today |
…4-BaseOptionsSelector-refactoring # Conflicts: # src/components/OptionsSelector/BaseOptionsSelector.js
|
@Piotrfj FYI: after Theme Switching migration we no longer import styles like this: |
…4-BaseOptionsSelector-refactoring # Conflicts: # src/components/OptionsSelector/BaseOptionsSelector.js
|
@thienlnam @shubham1206agra kindly asking for re-review 🙏🏻 |
| const subscribeToKeyboardShortcut = () => { | ||
| const enterConfig = CONST.KEYBOARD_SHORTCUTS.ENTER; | ||
| this.unsubscribeEnter = KeyboardShortcut.subscribe( | ||
| enterSubscription.current = KeyboardShortcut.subscribe( | ||
| enterConfig.shortcutKey, | ||
| this.selectFocusedOption, | ||
| selectFocusedOption, | ||
| enterConfig.descriptionKey, | ||
| enterConfig.modifiers, | ||
| true, | ||
| () => !this.state.allOptions[this.state.focusedIndex], | ||
| () => !allOptions[focusedIndex], | ||
| ); | ||
|
|
||
| const CTRLEnterConfig = CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER; | ||
| this.unsubscribeCTRLEnter = KeyboardShortcut.subscribe( | ||
| CTRLEnterSubscription.current = KeyboardShortcut.subscribe( | ||
| CTRLEnterConfig.shortcutKey, | ||
| () => { | ||
| if (this.props.canSelectMultipleOptions) { | ||
| this.props.onConfirmSelection(); | ||
| if (props.canSelectMultipleOptions) { | ||
| props.onConfirmSelection(); | ||
| return; | ||
| } | ||
|
|
||
| const focusedOption = this.state.allOptions[this.state.focusedIndex]; |
There was a problem hiding this comment.
Please use useKeyboardShortcut hook instead
|
@thienlnam Should we deprecate the use of shouldDelayFocus prop inside this component? Since we have |
|
@shubham1206agra @thienlnam I'm taking over this PR while @Piotrfj is ooo. If you'll request some other changes let me know 🙂 |
|
This is happening on staging as well. Approving. |
|
@thienlnam Can we please merge this before new conflicts come up again? |
|
The performance tests are failing - and it seems to be from this PR If I merge this, all PRs will start failing this check. @Piotrfj / @koko57 Are you able to take a look at this? |
|
@thienlnam looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.44-0 🚀
|
| const {translate} = useLocalize(); | ||
| const themeStyles = useThemeStyles(); | ||
|
|
||
| const getInitiallyFocusedIndex = useCallback( |
There was a problem hiding this comment.
In my experience, if you're using useCallback for a generator function that's only used once, you probably should use useMemo instead. I think that results in simpler code:
diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js
index 23451b8e1a..a182753387 100755
--- a/src/components/OptionsSelector/BaseOptionsSelector.js
+++ b/src/components/OptionsSelector/BaseOptionsSelector.js
@@ -58,27 +58,6 @@ function BaseOptionsSelector(props) {
const {translate} = useLocalize();
const themeStyles = useThemeStyles();
- const getInitiallyFocusedIndex = useCallback(
- (allOptions) => {
- let defaultIndex;
- if (props.shouldTextInputAppearBelowOptions) {
- defaultIndex = allOptions.length;
- } else if (props.focusedIndex >= 0) {
- defaultIndex = props.focusedIndex;
- } else {
- defaultIndex = props.selectedOptions.length;
- }
- if (_.isUndefined(props.initiallyFocusedOptionKey)) {
- return defaultIndex;
- }
-
- const indexOfInitiallyFocusedOption = _.findIndex(allOptions, (option) => option.keyForList === props.initiallyFocusedOptionKey);
-
- return indexOfInitiallyFocusedOption;
- },
- [props.shouldTextInputAppearBelowOptions, props.initiallyFocusedOptionKey, props.selectedOptions.length, props.focusedIndex],
- );
-
const isWebOrDesktop = [CONST.PLATFORM.DESKTOP, CONST.PLATFORM.WEB].includes(getPlatform());
const accessibilityRoles = _.values(CONST.ROLE);
@@ -137,7 +116,24 @@ function BaseOptionsSelector(props) {
// eslint-disable-next-line react-hooks/exhaustive-deps
const initialAllOptions = useMemo(() => flattenSections(), []);
const [allOptions, setAllOptions] = useState(initialAllOptions);
- const [focusedIndex, setFocusedIndex] = useState(getInitiallyFocusedIndex(initialAllOptions));
+
+ const initialFocusedIndex = useMemo(() => {
+ if (!_.isUndefined(props.initiallyFocusedOptionKey)) {
+ return _.findIndex(allOptions, (option) => option.keyForList === props.initiallyFocusedOptionKey);
+ }
+
+ let defaultIndex;
+ if (props.shouldTextInputAppearBelowOptions) {
+ defaultIndex = allOptions.length;
+ } else if (props.focusedIndex >= 0) {
+ defaultIndex = props.focusedIndex;
+ } else {
+ defaultIndex = props.selectedOptions.length;
+ }
+ return defaultIndex;
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- this value is only used to initialize state so only ever needs to be computed on the first render
+ }, []);
+ const [focusedIndex, setFocusedIndex] = useState(initialFocusedIndex);
const [focusedOption, setFocusedOption] = useState(allOptions[focusedIndex]);
/**
| } else if (props.focusedIndex >= 0) { | ||
| defaultIndex = props.focusedIndex; | ||
| } else { | ||
| defaultIndex = props.selectedOptions.length; |
There was a problem hiding this comment.
regression: if the length of selectedOptions is 0 then we get this bug #37239. In the diff I posted above, we default it to -1 instead of 0.
| [props.shouldTextInputAppearBelowOptions, props.initiallyFocusedOptionKey, props.selectedOptions.length, props.focusedIndex], | ||
| ); | ||
|
|
||
| const isWebOrDesktop = [CONST.PLATFORM.DESKTOP, CONST.PLATFORM.WEB].includes(getPlatform()); |
There was a problem hiding this comment.
I know you didn't add this, but it's not in compliance with our established code patterns for platform-specific code.
|
|
||
| setSections(newSections); | ||
| setAllOptions(newOptions); | ||
| setFocusedIndex(prevFocusedOptionIndex || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex)); |
There was a problem hiding this comment.
Since this was moved into an effect on sections, it's run on the initial render and incorrectly sets the focused index to 0 when it should be 1. That's causing this regression #37238.
More explanation posted here.
|
This seems to have caused this, @allroundexperts @Piotrfj in the future please if a base component is touched, please make sure to test all the pages that are using it. I haven't seen the NewChatPage in your recordings. |
|
Working on the regressions |
|
FYI this was reverted: #37454 |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀
|
|
Unfortunately, This PR was reverted becuase of multiple regressions |
Details
Fixed Issues
$ #16184
PROPOSAL: #16184 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Kapture.2024-01-09.at.16.41.22.mp4
MacOS: Desktop
Kapture.2024-01-10.at.23.25.12.mp4