Fix members page and invite page transition #25894
Conversation
melvin-bot
bot
requested review from
sobitneupane
and removed request for
a team
|
@sobitneupane 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] |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
@DrLoopFall Actually ... this somewhat illustrates that I should have asked for more detail before I accepted your proposal.
I was thinking something like this, which as I see it has several advantages over the implementation in this PR or the current implementation on main:
- It's simpler
- It is built-in to
SelectionListand thus will work every time it's used without the developer having to do anything extra at the screen level - It uses events built-in to react-navigation instead of potentially flaky
setTimeout
diff --git a/src/components/SelectionList/BaseSelectionList.js b/src/components/SelectionList/BaseSelectionList.js
index f14c3fef5d..fc7629a4db 100644
--- a/src/components/SelectionList/BaseSelectionList.js
+++ b/src/components/SelectionList/BaseSelectionList.js
@@ -1,7 +1,8 @@
-import React, {useEffect, useMemo, useRef, useState} from 'react';
+import React, {useEffect, useMemo, useRef, useState, useCallback} from 'react';
import {View} from 'react-native';
import _ from 'underscore';
import lodashGet from 'lodash/get';
+import {useNavigation} from '@react-navigation/native';
import SectionList from '../SectionList';
import Text from '../Text';
import styles from '../../styles/styles';
@@ -41,7 +42,6 @@ function BaseSelectionList({
keyboardType = CONST.KEYBOARD_TYPE.DEFAULT,
onChangeText,
initiallyFocusedOptionKey = '',
- shouldDelayFocus = false,
onScroll,
onScrollBeginDrag,
headerMessage = '',
@@ -52,10 +52,10 @@ function BaseSelectionList({
isKeyboardShown = false,
}) {
const {translate} = useLocalize();
+ const navigation = useNavigation();
const firstLayoutRef = useRef(true);
const listRef = useRef(null);
const textInputRef = useRef(null);
- const focusTimeoutRef = useRef(null);
const shouldShowTextInput = Boolean(textInputLabel);
const shouldShowSelectAll = Boolean(onSelectAll);
const shouldShowConfirmButton = Boolean(onConfirm);
@@ -268,23 +268,17 @@ function BaseSelectionList({
);
};
- /** Focuses the text input when the component mounts. If `props.shouldDelayFocus` is true, we wait for the animation to finish */
- useEffect(() => {
- if (shouldShowTextInput) {
- if (shouldDelayFocus) {
- focusTimeoutRef.current = setTimeout(() => textInputRef.current.focus(), CONST.ANIMATED_TRANSITION);
- } else {
+ /** Focuses the text input when the component comes into focus and the animation is finished. */
+ useEffect(
+ () =>
+ navigation.addListener('transitionEnd', () => {
+ if (!shouldShowTextInput) {
+ return;
+ }
textInputRef.current.focus();
- }
- }
-
- return () => {
- if (!focusTimeoutRef.current) {
- return;
- }
- clearTimeout(focusTimeoutRef.current);
- };
- }, [shouldDelayFocus, shouldShowTextInput]);
+ }),
+ [navigation, shouldShowTextInput],
+ );
/** Selects row when pressing enter */
useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
I tested and that seems to work well on all screens where SelectionList is used. Of course the shouldDelayFocus prop should be removed from all uses of SelectionList as well. Can you please make this change (unless you see some problem with it that I missed)?
|
I'm testing this, I'll update you once it's done. |
…nvite-members-page-25860
|
@roryabraham Currently, these pages are affected I thought making changes globally would cause bugs in other places, and that's why I made changes that won't affect other pages. What do you think? |
|
Alternatively, we can add a prop to the |
|
Hey all! So a quick note - on skimming this issue, I think this is going to be something that's going to be solved in the SelectionList refactor that's in progress, so I'm not sure it needs to be fixed separately. See the related discussion here |
|
Going to close this since it will be handled separately. @DrLoopFall should still be compensated for the fix. |
Details
Added an forwardRef in
BaseSelectionListwhich allows focusing the text input from outside the component.Added
autoFocusinBaseSelectionList, which defaults totrue, we can passfalse, to disable auto focusing, which breaks the animation.In
WorkspaceInvitePageandWorkspaceMembersPage, theautoFocusis set tofalse, and it's focused manually after the transition animation ends usingonEntryTransitionEnd, so the animation runs smoothly.Fixed Issues
$ #25860
PROPOSAL: #25860 (comment)
Tests
Membersand verify if the page animates/slides inInvitebutton and verify if the page animates/slides inOffline tests
N/A
QA Steps
Membersand verify if the page animates/slides inInvitebutton and verify if the page animates/slides inPR 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 */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)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
Web
web.mp4
Mobile Web - Chrome
android.chrome.mp4
Mobile Web - Safari
ios.safari.mp4
Desktop
App.mp4
iOS
ios.mp4
Android
android.mp4