Skip to content

Conversation

@elharony
Copy link
Contributor

@elharony elharony commented Oct 16, 2019

It's my first PR, so forgive me if it's not organized or something! 🙏

I found that we are resizing the Showcase section images using CSS, which isn't recommended. Instead, we need to Use Scaled Images directly. It seriously affects our website performance, here's GTMetrix Report:
React-Native-Website-On-GTMetrix

I resized all images (without decreasing its quality nor changing its extension) to 100x100. Total 14 images size was 574KB, and now it becomes 81.3KB (~86% smaller). This should give us a green Grade A on GTMetrix Serve Scaled Images criteria 👌

UPDATE: I changed them to 100x100 instead of 68x68 as it appears to be hardcoded to 100x100 on the Showcase Page, now it won't break the UI there.

@react-native-bot
Copy link

react-native-bot commented Oct 16, 2019

Deploy preview for react-native ready!

Built with commit 394b2dd

https://deploy-preview-1361--react-native.netlify.com

Changes to docs/ are reflected in the next "master" version.

Thank you for your contributions.

How to ContributeDocumentation Sources

@elharony elharony changed the title Replace current 14 'showcase' brands to scaled images (68x68) Replace current 14 'showcase' brands to scaled images (100x100) Oct 16, 2019
@rickhanlonii rickhanlonii self-assigned this Oct 16, 2019
@elharony elharony changed the title Replace current 14 'showcase' brands to scaled images (100x100) Replace 14 homepage 'showcase' brand images to scaled images (100x100) Oct 16, 2019
@rickhanlonii rickhanlonii removed their assignment Oct 16, 2019
@elharony
Copy link
Contributor Author

Any updates, @rickhanlonii ?

@rickhanlonii
Copy link
Member

@elharony, thanks for submitting this!

On the left is your change and the right is production:
Screen Shot 2019-11-11 at 9 55 55 PM

Switching to 100x100 causes blurring on retina screens, can you do this with 200x200 or 300x300? That should fix the blurring 👍

@elharony
Copy link
Contributor Author

Thanks @rickhanlonii for checking it out and providing such feedback!

We can't switch them to 300x300 as many of them is less than 300x300, but 200x200 is good for all 14 images, except the artsy, it's 167x167.

Well, I have a few suggestions here:

  1. Stick with 150x150, it will suit all current images & possibly future images as well.
  2. Keep them 200x200 except the artsy logo as it will look even worse if we scale it up (Bigger size, Bad quality!)

What do you think, Ricky? I would be happy to continue working on this part according to your feedback/suggestion to make it scalable for any future images as well.

@nearestnabors
Copy link
Contributor

What a wonderful idea! I have a suggestion: check out this format to accomodate retina screens as well as lower DPI without burdening one over the other:

<img 
  src="/img/2016/p_rachel-in-the-cubicle.png" 
  srcset="/img/2016/p_rachel-in-the-cubicle2x.png 2x" 
  height="338" width="600" alt="A manga-style illustration of Rachel Nabors winking from behind a computer monitor in an implied cubicle." 
/>

(From this old blog post)

You would use:

<img 
  src="/img/100x100.png" 
  srcset="/img/200x200.png 2x" 
  height="100" width="100" alt="Brand X" 
/>

Unless you don't have a 200x200 pic, in which case this works fine:

<img 
  src="/img/100x100.png" 
  height="100" width="100" alt="Brand X" 
/>

What do you all think?

@rickhanlonii
Copy link
Member

Sounds good to me! Sorry for the delay @elharony I must have missed the notification!

@elharony
Copy link
Contributor Author

Great. So, I will go for #1 Suggestion and keep the in 150x150, right @rickhanlonii ?

@rickhanlonii
Copy link
Member

Oh sorry, my message is confusing, I think we should go with @rachelnabors' solution which is your option 1, along with the updated image tags she provided 👍

@nearestnabors
Copy link
Contributor

Any movement on this @elharony? I'd soooo love to have this update on the site if you can get to it!

@rickhanlonii
Copy link
Member

rickhanlonii commented Dec 3, 2019

@elharony if you're not interesting in doing the full <img> tag version, updating this to use your option number 2 of using 200x200 for all images except artsy is good too!

@rickhanlonii
Copy link
Member

@orta do you know where we could find a artsy logo that's at least 200x200?

@elharony
Copy link
Contributor Author

Hey @rickhanlonii ,

Very sorry for being late, I am doing a Frontend Diploma with OpenClassrooms and I had some projects to finish. I've done the requested update, please give it another try!

@orta
Copy link
Contributor

orta commented Dec 18, 2019

@elharony
Copy link
Contributor Author

Thank you @orta . I've updated the Artsy logo.
@rickhanlonii Now, all of them are updated to 200x200 and optimized. Let me know if this PR needs any further changes

@rickhanlonii
Copy link
Member

Alright looks great thank you!

@rickhanlonii rickhanlonii merged commit 21137d0 into facebook:master Dec 26, 2019
@rickhanlonii
Copy link
Member

btw I just created this issue to update the Facebook logo if you're interested!

#1524

@elharony
Copy link
Contributor Author

Thanks for approving this PR. I am interested in #1524 and I will fix it! 👍

espipj pushed a commit to espipj/react-native-website that referenced this pull request Feb 8, 2020
facebook#1361)

* Replace current 14 'showcase' brands to scaled images (68x68)

* Resize the 14 images to be '100x100' instead of '68x68'

* perf: Optimize 14 showcase images (200x200)

* file: Add 200x200 'artsy' optimized logo
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