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.

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

Uploaded image preview placeholder #7569

Conversation

okansahin
Copy link
Contributor

@parasharrajat @JmillsExpensify

Details

Showing image preview until image uploaded.

Fixed Issues

$ #7463

Tests | QA Steps

  1. Open a chat window
  2. Click (+ button / add attachment / choose from gallery)
  3. Select an image
  4. Click Send button in preview modal
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Chrome
Screen.Recording.2022-02-04.at.12.44.48.mov
Safari
Screen.Recording.2022-02-04.at.12.45.48.mov

Mobile Web

Screen.Recording.2022-02-04.at.22.03.07.mov

Desktop

Screen.Recording.2022-02-04.at.14.12.33.mov

iOS

Screen.Recording.2022-02-04.at.21.43.22.mov

Android

Screen.Recording.2022-02-04.at.14.08.56.mov

@okansahin okansahin requested a review from a team as a code owner February 4, 2022 19:05
@MelvinBot MelvinBot requested review from Julesssss and parasharrajat and removed request for a team February 4, 2022 19:05
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@okansahin
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@parasharrajat
Copy link
Member

cc: @shawnborton Could you please review the design of the image preview?

@shawnborton
Copy link
Contributor

Is there a way we can make this more smooth? Could we detect the image dimensions upon upload and at least make it so that the preview area doesn't jump around as much?

Also, I'm curious about ActivityIndicator we're using here. Do we have other examples of where we use it in the app and it has a solid background behind it? I would think we would just show the loading spinner on top of the content with no box around it. Let me know if that makes sense.

@okansahin
Copy link
Contributor Author

@shawnborton

Is there a way we can make this more smooth? Could we detect the image dimensions upon upload and at least make it so that the preview area doesn't jump around as much?

Yes It can, but it is a different issue than this.

Also, I'm curious about ActivityIndicator we're using here. Do we have other examples of where we use it in the app and it has a solid background behind it? I would think we would just show the loading spinner on top of the content with no box around it. Let me know if that makes sense.

The activity indicator is default by operating system. I added a faded background because it's hard to see with photos which have light colors. (#7463 (comment))

If you provide me a design, I can set it to it.

@shawnborton
Copy link
Contributor

Why would you consider that a different issue than this?

@okansahin
Copy link
Contributor Author

okansahin commented Feb 7, 2022

Why would you consider that a different issue than this?

I thought this because we don't know the dimensions or ratio on the backend before uploaded. And doing this maybe needs changes that may affects the other parts of chat.

@parasharrajat Could you please read this comment and correct me if I'm wrong or please tell me if you have an idea.

I looked up code to how we can done the thing you want. I understand you because I'm keen attention the details too. But there are more complicated things than visible in this point.

The box of thumbnail image is already created by fixed dimensions(before image loaded) and It's 200x200.

constructor(props) {
super(props);
this.updateImageSize = this.updateImageSize.bind(this);
this.state = {
thumbnailWidth: 200,
thumbnailHeight: 200,
};
}

I have only two solution in my mind to solve jumping problem after loaded from backend,

  1. We can always show images full width, but it can affects images height especially images with long heights. If we don't care if it scrolls to bottom after image loaded the height can be fixed value.

Maybe it needs change the contentWidth in here:

<RenderHTMLSource
contentWidth={width * 0.8}
source={{html: props.html}}
/>

  1. And the other solution is to do the same dimension calculation in the backend on the frontend.

As for the ActivityIndicator design, if you provide me a design or express it precisely, I will try my best.

Thank you

@parasharrajat
Copy link
Member

Ok. yeah. Thanks for your thoughts @okansahin.

Is there a way we can make this more smooth? Could we detect the image dimensions upon upload and at least make it so that the preview area doesn't jump around as much?

I agree that this is not part of this PR and requires a bigger implementation change. But this is a good suggestion we can talk about this more on slack.

Also, I'm curious about ActivityIndicator we're using here. Do we have other examples of where we use it in the app and it has a solid background behind it? I would think we would just show the loading spinner on top of the content with no box around it. Let me know if that makes sense.

Let's try to work on this. @okansahin Do you have any suggestions?

@okansahin
Copy link
Contributor Author

okansahin commented Feb 7, 2022

Also, I'm curious about ActivityIndicator we're using here. Do we have other examples of where we use it in the app and it has a solid background behind it? I would think we would just show the loading spinner on top of the content with no box around it. Let me know if that makes sense.

Let's try to work on this. @okansahin Do you have any suggestions?

I thought this might be good for the ActivityIndicator to appear clear with every image.

If you think the same with me that faded background is required I might fill the faded background over image. But as @shawnborton said this usage is not exists with the other ActivityIndicators. It cause degradation of the design integrity.

We can use another spinner that you will choose, maybe with Lottie.

Or I can completely remove the faded background.

I think @shawnborton should decide this.

Thank you

@shawnborton
Copy link
Contributor

Okay that's a fair comment that we can follow up about dimensions in another PR, but what I found surprising with this animation is that we go from this shape:
image

Then we get a new shape and a blank preview (which we should definitely avoid):
image

Then we land on a final shape like this:
image

I think we should try to avoid the shape changing as much as we possibly can. Do you have any ideas for that?

For the ActivityIndicator, could we just try it without the background fill behind it?

@okansahin
Copy link
Contributor Author

okansahin commented Feb 7, 2022

I think we should try to avoid the shape changing as much as we possibly can. Do you have any ideas for that?

This catches my attention too. Even if we apply the solutions I wrote above, this will continue to happen. It requires intervention to the general structure in chat window. If you open a topic in Slack I will be glad to research and participate in the solution.

Even you don't upload a picture, this issue also happens when you open the app and click a chat room. (Here maybe retrieving images from the cache will fix the problem.)

For the ActivityIndicator, could we just try it without the background fill behind it?

I will do it in a short time and send the screenshots in here.

@okansahin
Copy link
Contributor Author

Simulator iOS

Screen Shot 2022-02-07 at 17 18 03

Desktop Chrome

Screen Shot 2022-02-07 at 17 24 24

@shawnborton
Copy link
Contributor

The spinner looks a bit too transparent on iOS, but looks fine in Chrome IMO.

This catches my attention too. Even if we apply the solutions I wrote above, this will continue to happen. It requires intervention to the general structure in chat window. If you open a topic in Slack I will be glad to research and participate in the solution.

Even you don't upload a picture, this issue also happens when you open the app and click a chat room. (Here maybe retrieving images from the cache will fix the problem.)

Hmm but I think where I am a bit confused is that in my post above, why does the second screenshot happen? Why can't we just go from 1 to 3 and this way the size only changes once?

@okansahin
Copy link
Contributor Author

okansahin commented Feb 7, 2022

The spinner looks a bit too transparent on iOS, but looks fine in Chrome IMO.

So, what do you want me to change. Is it okay like this?

This catches my attention too. Even if we apply the solutions I wrote above, this will continue to happen. It requires intervention to the general structure in chat window. If you open a topic in Slack I will be glad to research and participate in the solution.

Even you don't upload a picture, this issue also happens when you open the app and click a chat room. (Here maybe retrieving images from the cache will fix the problem.)

Hmm but I think where I am a bit confused is that in my post above, why does the second screenshot happen? Why can't we just go from 1 to 3 and this way the size only changes once?

Because it's about the way of loading messages. This "way" is not relational the code we have changed in this PR.

@shawnborton
Copy link
Contributor

Maybe for iOS, we can use our brand dark color for the base spinner color?

Because it's about the way of loading messages. This "way" is not relational the code we have changed in this PR.

That's fair, I think we should create a separate follow up issue to address that then.

@okansahin
Copy link
Contributor Author

okansahin commented Feb 8, 2022

Maybe for iOS, we can use our brand dark color for the base spinner color?

It looks like this with dark color which is defined in project. (#0b1b34)

Screen Shot 2022-02-08 at 11 09 03

@shawnborton
Copy link
Contributor

That definitely looks hard to see. Could we see it using one of our light gray colors?

@okansahin
Copy link
Contributor Author

That definitely looks hard to see. Could we see it using one of our light gray colors?

It's #FAFAFA which is applied.

Screen Shot 2022-02-08 at 11 24 34

@shawnborton
Copy link
Contributor

I think that looks better. What do you think?

@okansahin
Copy link
Contributor Author

Yes, it looks better.

I push code to repo if you agree with it.

@shawnborton
Copy link
Contributor

Yup, I agree.

@okansahin
Copy link
Contributor Author

@parasharrajat @Julesssss

waiting for you review. thank you.

@Julesssss
Copy link
Contributor

The iOS E2E tests failed with a timeout, just restarted them. Code looks pretty good to me other than that.

It would be good to make the follow-up improvements that yourself and Shawn discussed.

@okansahin
Copy link
Contributor Author

okansahin commented Feb 8, 2022

The iOS E2E tests failed with a timeout, just restarted them. Code looks pretty good to me other than that.

I saw that dotenv module not found in E2E test logs. Do I should merge main branch to this?

It would be good to make the follow-up improvements that yourself and Shawn discussed.

If you talking about for image jumping, I can create new bug request with that issue if you accept.

@Julesssss
Copy link
Contributor

Do I should merge main branch to this?

I didn't look into the errors, but if you think it will help then please do this.

@okansahin
Copy link
Contributor Author

I didn't look into the errors, but if you think it will help then please do this.

I didn't merged, all checks have passed now.

thank you @Julesssss

Comment on lines 31 to 34
name: PropTypes.string,
size: PropTypes.number,
type: PropTypes.string,
uri: PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

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

Please add comments to properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing quickly.

@@ -14,6 +14,7 @@ import withWindowDimensions, {windowDimensionsPropTypes} from '../../../componen
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import canUseTouchScreen from '../../../libs/canUseTouchscreen';
import compose from '../../../libs/compose';
import colors from '../../../styles/colors';
Copy link
Member

Choose a reason for hiding this comment

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

use variables instead of colors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked it if colors.js are used in components. And I saw in so many files with this usage.

Do you want me to add a new variable as a theme variable?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, Please use variables as much as possible for maintaince. Try to match existing variables if any make sense here otherwise you can create new.

added comments of attachmentInfo properties
@parasharrajat
Copy link
Member

parasharrajat commented Feb 8, 2022

Testing shortly... on my list 1/5.

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

image
Does this look fine @shawnborton? Or should we add a faded background over the image?

@shawnborton
Copy link
Contributor

I think that's probably fine. In a separate follow up issue, it would be nice to standardize on the same style spinner everywhere instead of having differences between platforms.

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM.

cc: @Julesssss
🎀 👀 🎀 C+ reviewed

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

LGTM

@Julesssss Julesssss merged commit 2a5b261 into Expensify:main Feb 9, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Feb 9, 2022

@okansahin, Great job getting your first Expensify/App pull request over the finish line! 🎉

I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:

  1. Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
  2. Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
  3. Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊

@OSBotify
Copy link
Contributor

OSBotify commented Feb 9, 2022

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

@OSBotify
Copy link
Contributor

OSBotify commented Feb 9, 2022

🚀 Deployed to staging by @Julesssss in version: 1.1.38-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.1.38-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Julesssss added a commit that referenced this pull request Feb 28, 2022
…e-preview-placeholder

Upload image preview placeholder - Follow-up #7569
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants