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

Add indicators to let the user know if they are on staging or dev #2506

Merged
merged 14 commits into from
Apr 27, 2021

Conversation

Jag96
Copy link
Contributor

@Jag96 Jag96 commented Apr 21, 2021

@roryabraham @shawnborton will you please review this?

Details

This PR adds visuals to the sign in screen, the main app screen, and the desktop icon to let the user know what environment they are running the app in (dev/staging/prod).

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/158445

Tests

  1. Run the app on dev on all platforms and sign out, confirm that you see a dev badge on the sign-in screen logo. On desktop, confirm you see a dev badge for the doc icon.
  2. Login to any account, confirm after logging in you see a dev badge on the main app screen next to Chats, and that the badge also shows up in Settings
  3. On desktop, run npm run desktop-build-staging, then open the Expensify.cash app inside dist/mac. Confirm the icon has the stg badge.
  4. Confirm on the sign-in screen the icon has a stg badge. Sign in and confirm the stg badge shows up next to Chats and Settings
  5. Update your .env file to include ENVIRONMENT=STAGING and run the app on web/ios/android, confirm you see the stg badge on the sign-in screen and in the app
  6. Update your .env file to include ENVIRONMENT=PRODUCTION and run the app on web/ios/android, confirm you see no badges on the sign-in screen and in the app

QA Steps

  1. Confirm that when running the app on staging, a STG badge appears on the sign in screen icon, and in the main app next to Chats and Settings (after login)
    2. Confirm that on desktop, the desktop icon shows with the STG badge
  2. Confirm that when running the app on prod, there are no badges

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web/Mobile Web/Desktop

Dev Staging Prod
image image image
image image image

iOS/Android

Dev Staging
image image
image image

@Jag96 Jag96 self-assigned this Apr 21, 2021
@Jag96 Jag96 requested a review from roryabraham April 22, 2021 00:16
@Jag96 Jag96 marked this pull request as ready for review April 22, 2021 00:16
@Jag96 Jag96 requested a review from a team as a code owner April 22, 2021 00:16
@MelvinBot MelvinBot requested review from ctkochan22 and removed request for a team April 22, 2021 00:17
// Prod and staging set the icon in the electron-builder config, so only update it here for dev
if (isDev) {
app.dock.setIcon(`${__dirname}/icon-dev.png`);
app.setName('Expensify.cash');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On dev, Electron is still shown in the dock and in the OS menu bar. https://www.electronjs.org/docs/tutorial/application-distribution had some examples on how to make this display properly on mac but I wasn't able to sort it out. Open to suggestions to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think that this is fine to leave on dev.

@@ -231,7 +231,6 @@ const styles = {

badge: {
backgroundColor: themeColors.badgeDefaultBG,
borderRadius: 14,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the IOUbadge, this border-radius is now set to 12 instead of 14 since I wanted to keep the increments of small/medium/large as multiples of 4 like we do for the spacing file. @shawnborton let me know if this should be set back to 14 and I can update the value in the new borders.js file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for badges, we want them to look like rounded pills and not rounded rectangles because buttons look like rounded rectangles. So maybe go the other way and jack this up to 16 or 20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's what 16 and 20 look like, I can update to either

16 20
image image

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I am not seeing a difference. Just to make sure we're on the same page, I think the style we are using for the $2.00 in your mockup is the general style we have for badges, and we should just reuse that style for the Chats header too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok sounds good, I'll revert the styles then so both badges have the old border-radius that the IOUBadge had

Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason the pill doesn't look perfectly rounded, but maybe my eyes are playing tricks on me. Can you double check that the "DEV" pill is only 20px tall? if that's the case, a border radius of > 10px should make it perfectly round.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the box model and element style for the DEV div
image image

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it just me, or is the text in the pill (both DEV and $2.00) not completely centered?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we see w/ borderRadius: 14?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it is with 14
image

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Just some minor comments, haven't tested yet

src/CONFIG.js Outdated Show resolved Hide resolved
src/components/EnvironmentBadge.js Outdated Show resolved Hide resolved
src/components/EnvironmentBadge.js Outdated Show resolved Hide resolved
src/components/EnvironmentBadge.js Outdated Show resolved Hide resolved
src/components/EnvironmentBadge.js Outdated Show resolved Hide resolved
src/components/ExpensifyCashLogo.js Outdated Show resolved Hide resolved
@@ -231,7 +231,6 @@ const styles = {

badge: {
backgroundColor: themeColors.badgeDefaultBG,
borderRadius: 14,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it just me, or is the text in the pill (both DEV and $2.00) not completely centered?

@@ -231,7 +231,6 @@ const styles = {

badge: {
backgroundColor: themeColors.badgeDefaultBG,
borderRadius: 14,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we see w/ borderRadius: 14?

roryabraham
roryabraham previously approved these changes Apr 23, 2021
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Okay, code LGTM 👍

@shawnborton I leave any remaining design-themed reviews in your capable hands 🙇🏽‍♂️

@Jag96
Copy link
Contributor Author

Jag96 commented Apr 26, 2021

@shawnborton pulling this out of the thread, here's the updated border-radius of 14 for both badges

shawnborton
shawnborton previously approved these changes Apr 26, 2021
@shawnborton
Copy link
Contributor

Looks good to me!

@roryabraham
Copy link
Contributor

@Jag96 Sorry bud, you've got conflicts 🙂

@Jag96
Copy link
Contributor Author

Jag96 commented Apr 27, 2021

Conflicts resolved

@roryabraham roryabraham self-requested a review April 27, 2021 01:36
@roryabraham
Copy link
Contributor

Nice!

image

image

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Cool. This PR just makes the staging / QA process feel so much more real for me. 😁

@roryabraham roryabraham merged commit da26cfe into main Apr 27, 2021
@roryabraham roryabraham deleted the joe-platform-icons branch April 27, 2021 01:47
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.31-2🚀

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

@isagoico
Copy link

iOS/Android - STG badge doesn't appear

Expected Result

An STG badge appears on the sign-in screen icon, and in the main app next to Chats and Settings (after login)

Actual Result

STG badge doesn't appear

Action performed

  1. Install test build 1.0.32-0
  2. Open sign-in screen
  3. Login
  4. Observe chat screen
  5. Open settings

Platform

Issue confirmed in:

iOS ✔️
Android ✔️

Notes/Images/Video

@Jag96
Copy link
Contributor Author

Jag96 commented Apr 27, 2021

@isagoico thanks! I'll create an issue and have a look, no need to block the deploy

@OSBotify
Copy link
Contributor

OSBotify commented May 8, 2021

🚀 Deployed to production in version: 1.0.39-5🚀

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

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