fix: 20585 - Web - Split Bill - Money request confirmation list stays highlighted after press #20838
Conversation
melvin-bot
bot
requested review from
Julesssss and
s77rt
and removed request for
a team
|
@s77rt @Julesssss One of you needs to 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] |
| hoverStyle={props.hoverStyle} | ||
| pressStyle={props.pressStyle} | ||
| focusStyle={props.focusStyle} |
There was a problem hiding this comment.
Curious why this wasn't being done previously...
There was a problem hiding this comment.
When creating these Pressable components I think we wanted to separate styles from GenericPressable and use them only in child component - OpacityView
There was a problem hiding this comment.
By passing them, to the parent component, we can easily fix the bug with blue borders
There was a problem hiding this comment.
Hmm I think the current implementation is the correct now, not sure why we did it that way but I think probably to support Button. This change breaks Button styles.
Didn't dive into the problem but seems like this should be fixed on OptionRow level.
@Skalakid I see you are not assigned on the linked issue yet. Were you assigned somewhere else?
There was a problem hiding this comment.
No, I wasn't assigned somewhere else, I've added the fix as it is a regression from my previous PR #19612 and @Christinadobrzyn asked me for some help.
Btw, can you provide more information on why this change brakes Button styles, please?
There was a problem hiding this comment.
The change itself is a breaking change, styles are no longer applied as they were. To reproduce the bug just hover any button e.g. Settings -> Payments -> "Add payment method" button
| Before | After |
|---|---|
 |
 |
| hoverStyle={props.hoverStyle} | ||
| pressStyle={props.pressStyle} | ||
| focusStyle={props.focusStyle} |
There was a problem hiding this comment.
I saw this in another PR, but I don't get why it is necessary now?
There was a problem hiding this comment.
Hmm good catch, I think we can just pass them with other props
|
I made changes in Nagranie.z.ekranu.2023-06-16.o.15.46.59.movThe problem is that we are focussing on pressable but the Nagranie.z.ekranu.2023-06-16.o.16.18.25.mov |
|
Bug: Buttons not being dimmed on press. |
src/components/OpacityView.js
Outdated
| <View style={StyleUtils.parseStyleAsArray(props.style)}>{props.children}</View> | ||
| </Animated.View> | ||
| ); | ||
| return <Animated.View style={[props.style, opacityStyle]}>{props.children}</Animated.View>; |
There was a problem hiding this comment.
This is against the styling guide. We should still parseStyleAsArray and deconstruct the styles.
<Animated.View style={[opacityStyle, ...StyleUtils.parseStyleAsArray(props.style)]}>{props.children}</Animated.View>;
Reviewer Checklist
Screenshots/VideosWebweb.mp4Mobile Web - Chromemweb-chrome.mp4Mobile Web - Safarimweb-safari.mp4Desktopdesktop.mp4iOSios.mp4Androidandroid.mp4 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.30-0 🚀
|
There was a problem hiding this comment.
@Skalakid Just found out we are doing something wrong here. Can you please raise a quick follow up to fix this.
- The rest props we pass to
GenericPressableshould be the first not to overwrite our props (onHoverIn, onPress, etc) - 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>There was a problem hiding this comment.
@s77rt I will handle this and create PR with the follow-up
There was a problem hiding this comment.
Hmm wasn't this a bug even before this PR? This PR hide some bugs (and some features) we had fix that in #21310
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.30-5 🚀
|
Details
Fixed Issues
$ #20585
PROPOSAL: #20585 (comment)
Tests
+button >New group> select some users)+button that is in theTextInputSplit billOffline tests
Same as Tests
QA Steps
Same as Tests
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 */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.mov
Mobile Web - Chrome
N/A
Mobile Web - Safari
N/A
Desktop
N/A
iOS
N/A
Android
N/A