#3019 move pointerEvents style from Image to View for Avatar #3129

dklymenk · 2021-05-25T19:24:18Z

@marcaaron Please check out the fix for https://github.com/Expensify/Expensify.cash/pull/3092/files#r639059422

Details

Since there is no pointerEvents style on react native Image, and we need to apply that style to a View instead, a new avatarWrapper style was created and all Avatars across the app now have a wrapping View with that style.

Fixed Issues

Fixes #3019

Tests

QA Steps

Mobile web Safari iOS:

On chat page tap on user icon to go to user details
Use 3D touch (long press on simulator) on the Profile icon
Make sure there is no image preview pop-up as in this video:

Bug5076490_Looking_weird.mp4

Other platforms:

On chat page tap on user icon to go to user details
Make sure the profile icon doesn't do anything unexpected when you long press it or try to interact with it in any other way.

Try doing the same for any avatar on the app. All the avatars should be covered by this PR, not just the one mentioned on the original issue #3019

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron

This looks OK, but would it be easier to just add a View around the Avatar component here... ?

https://github.com/Expensify/Expensify.cash/blob/9a9099592330bc26d06408f34d76128718196eb4/src/components/Avatar.js#L30-L36

dklymenk · 2021-05-25T19:48:43Z

@marcaaron You are right. It's definitely easier. I wasn't sure if that would be considered a good practice, to have an extra nested View just for that style, since in some places around the app, Avatar component already has a wrapping View.

If you believe I should create a view with that style inside Avatar component instead, please let me know.

There is one place where avatars are rendered, but actual Avatar component is not used in src/components/MultipleAvatars.js.

https://github.com/Expensify/Expensify.cash/blob/dc0c8b74ac04e3fd22865a3229743f0d270f7b43/src/components/MultipleAvatars.js#L48-L78

So I would need to include avatarWrapper style there somewhere anyway. Otherwise there is no real reason to do it outside Avatar component.

marcaaron · 2021-05-25T20:05:30Z

I wasn't sure if that would be considered a good practice, to have an extra nested View just for that style, since in some places around the app, Avatar component already has a wrapping View.

Good point. It could make things unnecessarily heavy, but I'm not sure if there are any bad effects. If we have mostly cases of Avatars with View around them an option could be to provide something like a containerStyle prop then removing the additional View wherever another Avatar is used instead opting to replace the extra View + styles...

e.g.

<View style={[styles.someStyle]}>
    <Avatar />
</View>

would be converted to

<Avatar containerStyles={[styles.someStyle]} />

marcaaron · 2021-05-25T20:06:57Z

Ultimately, I think what we want is to not have to remember to put an avatarWrapper style on every View containing an Avatar, since nobody would remember to do it.

dklymenk · 2021-05-25T20:12:25Z

The containerStyle prop sounds very good to me.

Ultimately, I think what we want is to not have to remember to put an avatarWrapper style on every View containing an Avatar, since nobody would remember to do it.

This is a very good point.

I'll convert the PR to draft and rework it.

Thanks.

dklymenk · 2021-05-25T20:49:49Z

@marcaaron

I've applied your suggestions. Named the prop containerStyle instead of containerStyles to be more in line with similar prop names across the app. Also made sure the new wrapper style is always there no matter what you pass with containerStyle.

Jag96

Nice simplification from the original PR, just one question

src/pages/DetailsPage.js

Jag96

LGTM, @marcaaron all yours

marcaaron

Looks good to me just had one minor comment.

marcaaron · 2021-05-26T18:08:20Z

src/components/Avatar.js

@@ -10,13 +10,17 @@ const propTypes = {
    /** Extra styles to pass */
    style: PropTypes.arrayOf(PropTypes.any),

+    /** Extra styles to pass to View wrapper */
+    containerStyle: PropTypes.arrayOf(PropTypes.any),


Can this be containerStyles since we are accepting an array of style objects?

Also should be PropTypes.arrayOf(PropTypes.object)

Named the prop containerStyle instead of containerStyles to be more in line with similar prop names across the app. Also made sure the new wrapper style is always there no matter what you pass with containerStyle.

There is a guide here on the correct usage -> https://github.com/Expensify/Expensify.cash/blob/main/STYLING.md#complex-component

Probably we need to update some other incorrect usages :)

There are at least ten props in the app that end with style (not styles) and most of them accept any. Do I change all the ones I could find? The way I see it, it's outside the scope of the issue, especially assuming I am expected to test each change.

I'll change the naming and proptype for the new containerStyle prop and I guess the existing style prop of Avatar component too, if that's ok.

No, please only change this specific instance.

If we see a style prop that means it can accept either an array or object. It's explained here.

Ok, does it mean that proptype should be

/** Additional styles to add after local styles */ style: PropTypes.oneOfType([ PropTypes.arrayOf(PropTypes.object), PropTypes.object, ]),

instead of PropTypes.arrayOf(PropTypes.any),

No, because this is not a style prop for a simple component, but specifically targets the wrapper element so we are looking for:

containerStyles: PropTypes.arrayOf(PropTypes.object)

Let me know if that makes sense or if the readme can be clearer about this somehow.

No, I mean the style prop, not containerStyles.

It was style: PropTypes.arrayOf(PropTypes.any), before I touched it, my question is should it be

style: PropTypes.oneOfType([ PropTypes.arrayOf(PropTypes.object), PropTypes.object, ]),

Since it's a simple style prop it should accept both object and array, as per styling guide.

Pushed an update. The way I see it, it's either like this or we rename the style prop to something like imageStyles and allow only array of objects there. If I understand the styling guidelines correctly, this how it is meant to be if you pass 2 style props: all of them should be plural, must have names describing the target element and accept only array of objects.

Yes, but we should not have a combination of style and containerStyles either we have multiple elements and we are getting specific or a single element and we are being vague.

It's totally fine if you don't want to make the change. We have some clean up to do and are still figuring some things out.

marcaaron

Sorry @dklymenk the pointerEvents should be on the View itself like this:

<View pointerEvents="none">

marcaaron · 2021-05-26T20:49:00Z

src/styles/styles.js

@@ -1030,6 +1028,10 @@ const styles = {
        borderColor: themeColors.sidebar,
    },

+    avatarWrapper: {
+        pointerEvents: 'none',
+    },


I think maybe the initial guidance was wrong because pointerEvents should be a prop on the View not a style

https://reactnative.dev/docs/view#pointerevents

dklymenk · 2021-05-27T11:30:04Z

@marcaaron
I have updated the PR with new style prop names and moved pointerEvents from style to props.

marcaaron

Awesome thanks for the changes!

OSBotify · 2021-05-27T19:19:53Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

dklymenk added 4 commits May 25, 2021 21:40

Expensify#3019 move pointerEvents style from Image to View

4896288

Expensify#3019 move pointerEvents style from Image to View

4341b0b

Expensify#3019 move pointerEvents style from Image to View

36cc498

Expensify#3019 move pointerEvents style from Image to View

5055c3d

dklymenk requested a review from a team as a code owner May 25, 2021 19:24

MelvinBot requested review from Jag96 and removed request for a team May 25, 2021 19:24

marcaaron reviewed May 25, 2021

View reviewed changes

dklymenk marked this pull request as draft May 25, 2021 20:12

dklymenk added 2 commits May 25, 2021 23:38

Expensify#3019 move pointerEvents style from Image to View

156362b

Expensify#3019 move pointerEvents style from Image to View

4b578c4

dklymenk marked this pull request as ready for review May 25, 2021 20:46

Jag96 reviewed May 25, 2021

View reviewed changes

src/pages/DetailsPage.js Outdated Show resolved Hide resolved

Expensify#3019 remove redundant styles.avatarWrapper

37bc631

Jag96 previously approved these changes May 26, 2021

View reviewed changes

marcaaron requested changes May 26, 2021

View reviewed changes

Expensify#3019 change styles prop naming to plural

4af5cad

dklymenk dismissed Jag96’s stale review via 4af5cad May 26, 2021 18:33

dklymenk requested a review from marcaaron May 26, 2021 18:33

Expensify#3019 apply styling guidelines

0ac88b1

marcaaron requested changes May 26, 2021

View reviewed changes

dklymenk added 3 commits May 27, 2021 13:57

Expensify#3019 move pointerEvents none to prop

909c279

Expensify#3019 rename Avatar style prop to imageStyles

03e8e3d

Expensify#3019 remove leftover reference to avatarWrapper style

8437a6d

dklymenk requested a review from marcaaron May 27, 2021 11:30

marcaaron approved these changes May 27, 2021

View reviewed changes

marcaaron merged commit b4c8d11 into Expensify:main May 27, 2021

marcaaron assigned dklymenk May 27, 2021

dklymenk deleted the 3019-move-pointer-events-style-to-view branch May 27, 2021 19:21

OSBotify mentioned this pull request May 28, 2021

Deploy Checklist: Expensify.cash 2021-05-28 #3199

Closed

27 tasks

bernhardoj mentioned this pull request Jun 30, 2023

[HOLD for payment 2023-08-02] [$1000] IOS - Long Pressing on Attachment in chat history in Safari opens the native image menu #21344

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3019 move pointerEvents style from Image to View for Avatar #3129

#3019 move pointerEvents style from Image to View for Avatar #3129

dklymenk commented May 25, 2021

marcaaron left a comment

dklymenk commented May 25, 2021

marcaaron commented May 25, 2021

marcaaron commented May 25, 2021

dklymenk commented May 25, 2021

dklymenk commented May 25, 2021

Jag96 left a comment

Jag96 left a comment

marcaaron left a comment

marcaaron May 26, 2021

marcaaron May 26, 2021

dklymenk May 26, 2021

marcaaron May 26, 2021

dklymenk May 26, 2021 •

edited

Loading

marcaaron May 26, 2021

dklymenk May 26, 2021

dklymenk May 26, 2021

marcaaron May 26, 2021

marcaaron left a comment

marcaaron May 26, 2021

dklymenk commented May 27, 2021

marcaaron left a comment

OSBotify commented May 27, 2021

#3019 move pointerEvents style from Image to View for Avatar #3129

#3019 move pointerEvents style from Image to View for Avatar #3129

Conversation

dklymenk commented May 25, 2021

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron left a comment

Choose a reason for hiding this comment

dklymenk commented May 25, 2021

marcaaron commented May 25, 2021

marcaaron commented May 25, 2021

dklymenk commented May 25, 2021

dklymenk commented May 25, 2021

Jag96 left a comment

Choose a reason for hiding this comment

Jag96 left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dklymenk May 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dklymenk commented May 27, 2021

marcaaron left a comment

Choose a reason for hiding this comment

OSBotify commented May 27, 2021

dklymenk May 26, 2021 •

edited

Loading