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

Updates NewDot homepage to reflect N6 campaign #5318

Merged
merged 8 commits into from
Oct 15, 2021
Merged

Updates NewDot homepage to reflect N6 campaign #5318

merged 8 commits into from
Oct 15, 2021

Conversation

Gonals
Copy link
Contributor

@Gonals Gonals commented Sep 17, 2021 •
edited
Loading

cc @shawnborton, @yuwenmemon
@MonilBhavsar, @HorusGoul 🐻‍❄️ (Although you may want to wait until Shawn gives his two cents. My CSS skills are very lacking 😬)

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/176436

Tests

  1. Open newdot. See the new image.
  2. Refresh a few times. Check that the image randomly rotates between 4 colors.

QA Steps

  1. Open newdot. See the new image.
  2. Refresh a few times. Check that the image randomly rotates between 4 colors.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-09-21 at 5 56 44 PM

Desktop

Screen Shot 2021-09-21 at 5 55 25 PM

iOS

Not displayed on mobiles

Android

Not displayed on mobiles

@Gonals Gonals requested review from shawnborton and a team September 17, 2021 12:22
@Gonals Gonals self-assigned this Sep 17, 2021
@MelvinBot MelvinBot requested review from HorusGoul and removed request for a team September 17, 2021 12:22
@Gonals Gonals requested a review from a team as a code owner September 17, 2021 12:22
@MelvinBot MelvinBot requested review from MonilBhavsar and removed request for a team September 17, 2021 12:22
HorusGoul
HorusGoul suggested changes Sep 17, 2021
View reviewed changes
Copy link
Contributor

@HorusGoul HorusGoul left a comment

Choose a reason for hiding this comment

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

These backgrounds look great!

Regarding the code:

  • We should add an alt to these images for accessibility.

  • And we could optimize the SVGs using SVGOMG or a similar tool to improve load times.

Let me know what you think.

@Gonals
Copy link
Contributor Author

Gonals commented Sep 20, 2021

These backgrounds look great!

Regarding the code:

  • We should add an alt to these images for accessibility.
  • And we could optimize the SVGs using SVGOMG or a similar tool to improve load times.

Let me know what you think.

Hey!
Looking around the code, it doesn't look like we consistently add alt. Since this image isn't really relevant for usage, I don't think we need one.

As for the load time optimizer, I think it could be a good idea to discuss it somewhere on Slack and see if this is something we want to use across the app. I think it is a bit out of scope for this PR, but it could certainly be interesting as an app-wide initiative.

@Gonals
Copy link
Contributor Author

Gonals commented Sep 20, 2021

Comments answered!

@shawnborton
Copy link
Contributor

Can you share some more screenshots at various screen widths? It also looks like the text might not be vertically centered as well.

@Gonals
Copy link
Contributor Author

Gonals commented Sep 21, 2021

Can you share some more screenshots at various screen widths? It also looks like the text might not be vertically centered as well.

@shawnborton It may very well be the case. I've been having trouble centering it since the image's proportions don't really fit the screen's. Would it be possible to simply get the text with a clear background? That way I can easily center it and I'll just rotate between the images and background colors.

@Gonals
Copy link
Contributor Author

Gonals commented Sep 21, 2021

A few examples:
Screen Shot 2021-09-21 at 10 28 53 AM
Screen Shot 2021-09-21 at 10 29 26 AM
Screen Shot 2021-09-21 at 10 29 40 AM

@shawnborton
Copy link
Contributor

Well the thing with centering it is that then the image itself won't scale proportionally. Were you able to get it working with object-fit: cover or as a background image using background-size: cover?

@Gonals
Copy link
Contributor Author

Gonals commented Sep 21, 2021 •
edited
Loading

Well the thing with centering it is that then the image itself won't scale proportionally. Were you able to get it working with object-fit: cover or as a background image using background-size: cover?

It doesn't look like we can set .svg as images/background images. They are sort of their own components in react-native 🤷

@Gonals
Copy link
Contributor Author

Gonals commented Sep 21, 2021 •
edited
Loading

Well the thing with centering it is that then the image itself won't scale proportionally. Were you able to get it working with object-fit: cover or as a background image using background-size: cover?

It doesn't look like we can set .svg as images/background images. They are sort of their own components in react-native 🤷

Nevermind! I was wrong. They can't be images, but they can be background-images (weird)! Changes incoming!

@shawnborton
Copy link
Contributor

Great news!

@Gonals
Copy link
Contributor Author

Gonals commented Sep 21, 2021

Sooo... Not looking great. Works well on web, but on desktop:
Without cover:
Screen Shot 2021-09-21 at 4 52 27 PM

With cover:
Screen Shot 2021-09-21 at 5 53 06 PM

@shawnborton
Copy link
Contributor

Hmm got it. So then what happens if you go back to your original implementation where you used background colors, but we just vertically and horizontally center the image, and make sure the image has a max-width of 100% and a max-height of 100%?

@Gonals
Copy link
Contributor Author

Gonals commented Sep 21, 2021 •
edited
Loading

I did something similar. I set the background image size to 100% 100% and also set the background color:
Screen Shot 2021-09-21 at 5 55 25 PM
Screen Shot 2021-09-21 at 5 56 44 PM

@Gonals
Copy link
Contributor Author

Gonals commented Sep 21, 2021 •
edited
Loading

Some resizes:
Screen Shot 2021-09-21 at 6 08 00 PM
Screen Shot 2021-09-21 at 6 08 10 PM

@shawnborton
Copy link
Contributor

Sweet, I say we go with that then - I think it will end up looking good in most situations.

@Gonals
Copy link
Contributor Author

Gonals commented Sep 21, 2021

Coolio! This should be ready for code-review, then!

@yuwenmemon yuwenmemon dismissed stale reviews from HorusGoul, MonilBhavsar, and themself via 4339b0b October 15, 2021 18:55
@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@yuwenmemon
Copy link
Contributor

Since I added a commit, getting a puller bear to review.

@yuwenmemon yuwenmemon requested a review from a team October 15, 2021 18:56
@MelvinBot MelvinBot requested review from HorusGoul and removed request for a team October 15, 2021 18:57
@yuwenmemon yuwenmemon requested review from a team and removed request for HorusGoul October 15, 2021 18:57
@MelvinBot MelvinBot requested review from HorusGoul and removed request for a team October 15, 2021 18:58
@yuwenmemon yuwenmemon requested review from luacmartins and removed request for HorusGoul October 15, 2021 19:00
luacmartins
luacmartins approved these changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

LGTM and tested well!

@luacmartins luacmartins merged commit abed3f8 into main Oct 15, 2021
@luacmartins luacmartins deleted the alberto-brandImages branch October 15, 2021 19:45
github-actions bot pushed a commit that referenced this pull request Oct 15, 2021
@luacmartins
Merge pull request #5318 from Expensify/alberto-brandImages
16342fe 
Updates NewDot homepage to reflect N6 campaign

(cherry picked from commit abed3f8)
This was referenced Oct 15, 2021
Merged
Closed
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @luacmartins in version: 1.1.8-1 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@isagoico
Copy link

Found this issue that's related to this PR - #5912

@ogumen
Copy link

ogumen commented Oct 21, 2021

The text displayed in the right side of the screen is not conveyed to screen reader users. It is implemented as a background image of text which is a failure of WCAG success criteria 1.4.5.

@OSBotify OSBotify mentioned this pull request Oct 23, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @luacmartins in version: 1.1.8-10 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

@OSBotify OSBotify mentioned this pull request Aug 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luacmartins luacmartins luacmartins approved these changes

@MonilBhavsar MonilBhavsar MonilBhavsar left review comments

@HorusGoul HorusGoul HorusGoul left review comments

@yuwenmemon yuwenmemon yuwenmemon left review comments

@shawnborton shawnborton Awaiting requested review from shawnborton

Assignees

@Gonals Gonals

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.