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.

Sign up for GitHub

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: prevent refocusing in options-selector and selection-list #29084

Merged
merged 7 commits into from
Oct 10, 2023
Merged

fix: prevent refocusing in options-selector and selection-list #29084

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e41454b
fix: prevent refocusing in selection-list
sangar-1028 Oct 6, 2023
5142adc
fix: prevent refocusing in option-selector
sangar-1028 Oct 6, 2023
1f59c8f
Merge branch 'main' into fix/issue-22812
sangar-1028 Oct 9, 2023
5a74195
fix: change props name
sangar-1028 Oct 9, 2023
90c6039
fix: prettier issue
sangar-1028 Oct 9, 2023
2b2d137
fix: focus in workspace invite page
sangar-1028 Oct 9, 2023
1b94af4
fix: use unique props name
sangar-1028 Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/components/OptionRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ const propTypes = {
/** Whether to remove the lateral padding and align the content with the margins */
shouldDisableRowInnerPadding: PropTypes.bool,

/** Whether to take focus when selecting */
shouldTakeFocus: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make the name consistent? So use shouldFocusOnSelectRow and reverse conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can, but it looks weird considering the name shouldFocusOnSelectRow as OptionRow's props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@situchan

When we should refocus on selecting row for options-selector, the option row should not take focus. I thought this makes sense.
Do you still want to rename the props?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, tricky naming. shoudTakeFocus is fine but no one knows what this does when read this code without any context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe shouldPreventDefaultFocusOnClick though it's long name?

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldPreventDefaultFocusOnSelect is comparatively near to what we do in the code. So I would vote for it first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we still want to have the same props name, I suggest shouldPreventDefaultFocusOnSelectRow

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldPreventDefaultFocusOnSelectRow sounds good to me.
I think also shouldPreventDefaultFocusOnSelectItem will work.
But yes, using same prop through out the tree will be better IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Let me rename the props for BaseOptionSelector, BaseOptionsList, BaseSelectionList, BaseListItem, and OptionRow

cc @situchan

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Let me rename the props for BaseOptionSelector, BaseOptionsList, BaseSelectionList, BaseListItem, and OptionRow

cc @situchan

👍


/** Whether to wrap large text up to 2 lines */
isMultilineSupported: PropTypes.bool,

Expand All @@ -95,6 +98,7 @@ const defaultProps = {
style: null,
shouldHaveOptionSeparator: false,
shouldDisableRowInnerPadding: false,
shouldTakeFocus: true,
};

class OptionRow extends Component {
Expand Down Expand Up @@ -213,6 +217,7 @@ class OptionRow extends Component {
hoverDimmingValue={1}
hoverStyle={this.props.hoverStyle}
needsOffscreenAlphaCompositing={lodashGet(this.props.option, 'icons.length', 0) >= 2}
onMouseDown={this.props.shouldTakeFocus ? undefined : (e) => e.preventDefault()}
>
<View style={sidebarInnerRowStyle}>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
Expand Down
2 changes: 2 additions & 0 deletions src/components/OptionsList/BaseOptionsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ function BaseOptionsList({
showScrollIndicator,
listContainerStyles,
shouldDisableRowInnerPadding,
shouldTakeFocus,
disableFocusOptions,
canSelectMultipleOptions,
shouldShowMultipleOptionSelectorAsButton,
Expand Down Expand Up @@ -206,6 +207,7 @@ function BaseOptionsList({
isDisabled={isItemDisabled}
shouldHaveOptionSeparator={index > 0 && shouldHaveOptionSeparator}
shouldDisableRowInnerPadding={shouldDisableRowInnerPadding}
shouldTakeFocus={shouldTakeFocus}
isMultilineSupported={isRowMultilineSupported}
/>
);
Expand Down
4 changes: 4 additions & 0 deletions src/components/OptionsList/optionsListPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ const propTypes = {
/** Whether to disable the inner padding in rows */
shouldDisableRowInnerPadding: PropTypes.bool,

/** Whether to take focus when selecting a row */
shouldTakeFocus: PropTypes.bool,

/** Whether to show the scroll bar */
showScrollIndicator: PropTypes.bool,

Expand Down Expand Up @@ -107,6 +110,7 @@ const defaultProps = {
onLayout: undefined,
shouldHaveOptionSeparator: false,
shouldDisableRowInnerPadding: false,
shouldTakeFocus: true,
showScrollIndicator: false,
isRowMultilineSupported: false,
};
Expand Down
1 change: 1 addition & 0 deletions src/components/OptionsSelector/BaseOptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ class BaseOptionsSelector extends Component {
isLoading={!this.props.shouldShowOptions}
showScrollIndicator={this.props.showScrollIndicator}
isRowMultilineSupported={this.props.isRowMultilineSupported}
shouldTakeFocus={!this.props.shouldFocusOnSelectRow}
/>
);
return (
Expand Down
3 changes: 2 additions & 1 deletion src/components/SelectionList/BaseListItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import RadioListItem from './RadioListItem';
import OfflineWithFeedback from '../OfflineWithFeedback';
import CONST from '../../CONST';

function BaseListItem({item, isFocused = false, isDisabled = false, showTooltip, canSelectMultiple = false, onSelectRow, onDismissError = () => {}}) {
function BaseListItem({item, isFocused = false, isDisabled = false, showTooltip, shouldTakeFocus = true, canSelectMultiple = false, onSelectRow, onDismissError = () => {}}) {
const isUserItem = lodashGet(item, 'icons.length', 0) > 0;
const ListItem = isUserItem ? UserListItem : RadioListItem;

Expand All @@ -32,6 +32,7 @@ function BaseListItem({item, isFocused = false, isDisabled = false, showTooltip,
hoverDimmingValue={1}
hoverStyle={styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
onMouseDown={shouldTakeFocus ? undefined : (e) => e.preventDefault()}
>
<View
style={[
Expand Down
2 changes: 2 additions & 0 deletions src/components/SelectionList/BaseSelectionList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ function BaseSelectionList({
canSelectMultiple={canSelectMultiple}
onSelectRow={() => selectRow(item, true)}
onDismissError={onDismissError}
shouldTakeFocus={!shouldFocusOnSelectRow}
/>
);
};
Expand Down Expand Up @@ -401,6 +402,7 @@ function BaseSelectionList({
accessibilityState={{checked: flattenedSections.allSelected}}
disabled={flattenedSections.allOptions.length === flattenedSections.disabledOptionsIndexes.length}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
onMouseDown={shouldFocusOnSelectRow ? (e) => e.preventDefault() : undefined}
>
<Checkbox
accessibilityLabel={translate('workspace.people.selectAll')}
Expand Down
1 change: 1 addition & 0 deletions src/components/SelectionList/selectionListPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ const radioListItemPropTypes = {
const baseListItemPropTypes = {
...commonListItemPropTypes,
item: PropTypes.oneOfType([PropTypes.shape(userListItemPropTypes.item), PropTypes.shape(radioListItemPropTypes.item)]),
shouldTakeFocus: PropTypes.bool,
};

const propTypes = {
Expand Down
Loading