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

Jump to bottom

fix: 20585 - Web - Split Bill - Money request confirmation list stays highlighted after press #20838

Merged
merged 6 commits into from
Jun 20, 2023
Merged

fix: 20585 - Web - Split Bill - Money request confirmation list stays highlighted after press #20838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1838ebe
fixed issue with items staying higlighted
Skalakid Jun 15, 2023
52e4490
fixed bugs with styling after PressableWithFeedback structure change
Skalakid Jun 16, 2023
21c6155
fixed passing onHoverIn, onHoverOut etc functions to Pressable
Skalakid Jun 16, 2023
af6a561
removed ommiting pressable styles
Skalakid Jun 16, 2023
ac866f5
fixed bug with dimming
Skalakid Jun 19, 2023
2c379ca
added parseStyleAsArray to styles
Skalakid Jun 19, 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
9 changes: 2 additions & 7 deletions src/components/OpacityView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import React from 'react';
import {View} from 'react-native';
import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated';
import PropTypes from 'prop-types';
import * as StyleUtils from '../styles/StyleUtils';
import variables from '../styles/variables';
import * as StyleUtils from '../styles/StyleUtils';

const propTypes = {
/**
Expand Down Expand Up @@ -49,11 +48,7 @@ function OpacityView(props) {
}
}, [props.shouldDim, props.dimmingValue, opacity]);

return (
<Animated.View style={[opacityStyle]}>
<View style={StyleUtils.parseStyleAsArray(props.style)}>{props.children}</View>
</Animated.View>
);
return <Animated.View style={[opacityStyle, ...StyleUtils.parseStyleAsArray(props.style)]}>{props.children}</Animated.View>;
}

OpacityView.displayName = 'OpacityView';
Expand Down
1 change: 0 additions & 1 deletion src/components/OptionRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ class OptionRow extends Component {
accessibilityRole="button"
hoverDimmingValue={1}
hoverStyle={this.props.hoverStyle}
focusStyle={this.props.hoverStyle}
>
<View style={sidebarInnerRowStyle}>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
Expand Down
81 changes: 45 additions & 36 deletions src/components/Pressable/PressableWithFeedback.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

@Skalakid Just found out we are doing something wrong here. Can you please raise a quick follow up to fix this.

  1. The rest props we pass to GenericPressable should be the first not to overwrite our props (onHoverIn, onPress, etc)
  2. The omittedProps should only be wrapperStyle

Something like that

diff --git a/src/components/Pressable/PressableWithFeedback.js b/src/components/Pressable/PressableWithFeedback.js
index 2949683734..3defd7d640 100644
--- a/src/components/Pressable/PressableWithFeedback.js
+++ b/src/components/Pressable/PressableWithFeedback.js
@@ -7,7 +7,7 @@ import GenericPressablePropTypes from './GenericPressable/PropTypes';
 import OpacityView from '../OpacityView';
 import variables from '../../styles/variables';
 
-const omittedProps = ['wrapperStyle', 'onHoverIn', 'onHoverOut', 'onPressIn', 'onPressOut'];
+const omittedProps = ['wrapperStyle'];
 
 const PressableWithFeedbackPropTypes = {
     ...GenericPressablePropTypes.pressablePropTypes,
@@ -55,6 +55,8 @@ const PressableWithFeedback = forwardRef((props, ref) => {
         >
             <GenericPressable
                 ref={ref}
+                // eslint-disable-next-line react/jsx-props-no-spreading
+                {...rest}
                 disabled={disabled}
                 onHoverIn={() => {
                     setIsHovered(true);
@@ -85,8 +87,6 @@ const PressableWithFeedback = forwardRef((props, ref) => {
                         });
                     });
                 }}
-                // eslint-disable-next-line react/jsx-props-no-spreading
-                {...rest}
             >
                 {(state) => (_.isFunction(props.children) ? props.children(state) : props.children)}
             </GenericPressable>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt I will handle this and create PR with the follow-up

Copy link
Contributor

Choose a reason for hiding this comment

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

@Skalakid Just wanted to let you know that this proposal created a regression: #21354

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm wasn't this a bug even before this PR? This PR hide some bugs (and some features) we had fix that in #21310

Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@ import GenericPressable from './GenericPressable';
import GenericPressablePropTypes from './GenericPressable/PropTypes';
import OpacityView from '../OpacityView';
import variables from '../../styles/variables';
import * as StyleUtils from '../../styles/StyleUtils';

const omittedProps = ['style', 'pressStyle', 'hoverStyle', 'focusStyle', 'wrapperStyle'];
const omittedProps = ['wrapperStyle', 'onHoverIn', 'onHoverOut', 'onPressIn', 'onPressOut'];

const PressableWithFeedbackPropTypes = {
..._.omit(GenericPressablePropTypes.pressablePropTypes, omittedProps),
Expand Down Expand Up @@ -39,49 +38,59 @@ const PressableWithFeedbackDefaultProps = {
};

const PressableWithFeedback = forwardRef((props, ref) => {
const propsWithoutStyling = _.omit(props, omittedProps);
const rest = _.omit(props, omittedProps);
const [disabled, setDisabled] = useState(props.disabled);
const [isPressed, setIsPressed] = useState(false);
const [isHovered, setIsHovered] = useState(false);

useEffect(() => {
setDisabled(props.disabled);
}, [props.disabled]);

return (
<GenericPressable
ref={ref}
<OpacityView
shouldDim={Boolean(!disabled && (isPressed || isHovered))}
dimmingValue={isPressed ? props.pressDimmingValue : props.hoverDimmingValue}
style={props.wrapperStyle}
// eslint-disable-next-line react/jsx-props-no-spreading
{...propsWithoutStyling}
disabled={disabled}
onPress={(e) => {
setDisabled(true);
const onPress = props.onPress(e);
InteractionManager.runAfterInteractions(() => {
if (!(onPress instanceof Promise)) {
setDisabled(props.disabled);
return;
}
onPress.finally(() => {
setDisabled(props.disabled);
});
});
}}
>
{(state) => (
<OpacityView
shouldDim={Boolean(!disabled && (state.pressed || state.hovered))}
dimmingValue={state.pressed ? props.pressDimmingValue : props.hoverDimmingValue}
style={[
...StyleUtils.parseStyleFromFunction(props.style, state),
...(!disabled && state.pressed ? StyleUtils.parseStyleFromFunction(props.pressStyle, state) : []),
...(!disabled && state.hovered ? StyleUtils.parseStyleAsArray(props.hoverStyle, state) : []),
...(state.focused ? StyleUtils.parseStyleAsArray(props.focusStyle, state) : []),
]}
>
{_.isFunction(props.children) ? props.children(state) : props.children}
</OpacityView>
)}
</GenericPressable>
<GenericPressable
ref={ref}
disabled={disabled}
onHoverIn={() => {
setIsHovered(true);
if (props.onHoverIn) props.onHoverIn();
}}
onHoverOut={() => {
setIsHovered(false);
if (props.onHoverOut) props.onHoverOut();
}}
onPressIn={() => {
setIsPressed(true);
if (props.onPressIn) props.onPressIn();
}}
onPressOut={() => {
setIsPressed(false);
if (props.onPressOut) props.onPressOut();
}}
onPress={(e) => {
setDisabled(true);
const onPress = props.onPress(e);
InteractionManager.runAfterInteractions(() => {
if (!(onPress instanceof Promise)) {
setDisabled(props.disabled);
return;
}
onPress.finally(() => {
setDisabled(props.disabled);
});
});
}}
// eslint-disable-next-line react/jsx-props-no-spreading
{...rest}
>
{(state) => (_.isFunction(props.children) ? props.children(state) : props.children)}
</GenericPressable>
</OpacityView>
);
});

Expand Down