Workspace Editor Page Design Updates #12419

dangrous · 2022-11-03T00:46:02Z

Details

Fixed Issues

Tests

See QA Steps

Verify that no errors appear in the JS console

QA Steps

Note: I'm sorry in advance. The testing steps are the same on all platforms. There should be no changes to functionality with any of these code changes. I'm a little unsure if these will actually get to all the screens - I had to hack a bit locally to make it work - so please reach out to me if you need assistance and we can work through it together.

Log into an account without a VBA.
- Create a new Workspace and go to the Workspace Settings
  - Click the question mark in the top right of the Workspace Settings view and confirm that the design matches 1 in the screenshots file, and all buttons/links work as expected.
    - Go to Request a setup call and confirm that the design matches 2 in the screenshots file, and all buttons/links work as expected.
  - Go to Issue cards and confirm that the design matches 10 in the screenshots file, and all buttons/links work as expected.
  - Go to Reimburse expenses and confirm that the design matches 16, 17, and 19 in the screenshots file, and all buttons/links work as expected.
  - Go to Pay bills and confirm that the design matches 7 and 8 in the screenshots file, and all buttons/links work as expected.
  - Go to Send invoices and confirm that the design matches 13 and 14 in the screenshots file, and all buttons/links work as expected.
  - Go to Book travel and confirm that the design matches 20 in the screenshots file, and all buttons/links work as expected.
  - Go to Connect bank account and confirm that the design matches 3 in the screenshots file, and all buttons/links work as expected.
  - Connect a bank account via Plaid and continue to the end of the flow, where it asks you to add a work email address. Confirm that the design matches 5 in the screenshots file, and all buttons/links work as expected
  - Cancel adding that bank account and restart the connection manually.
  - When you get to the flow asking for verification amounts, leave the flow.
  - Go back into Workspace settings and click Connect bank account again. Confirm that the design matches 4 in the screenshots file, and all buttons/links work as expected.
  - Return to the validation screen and enter in the amounts and continue. Confirm that the design matches 6 in the screenshots file, and all buttons/links work as expected.
Log into an account/workspace with a VBA.
- Go to the Workspace Settings
  - Go to Issue cards and confirm that the design matches 11 in the screenshots file, and all buttons/links work as expected.
  - Go to Reimburse expenses and confirm that the design matches 16, 17, and 18 in the screenshots file, and all buttons/links work as expected.
  - Go to Pay bills and confirm that the design matches 7 and 9 in the screenshots file, and all buttons/links work as expected.
  - Go to Send invoices and confirm that the design matches 13 and 15 in the screenshots file, and all buttons/links work as expected.
  - Go to Book travel and confirm that the design matches 21 in the screenshots file, and all buttons/links work as expected.
Log into an account/workspace with a VBA and an Expensify Card
- Go to the Workspace settings
- Go to Issue cards and confirm that the design matches 12 in the screenshots file, and all buttons/links work as expected.
Log into an account/workspace with an Expensify Card.
- Go to the Workspace Settings
- Go to Connect a bank account and go through the flow adding a bank account via Plaid.
- After verifying your personal information, confirm that the design matches 22 in the screenshots file, and all buttons/links work as expected.

Verify that no errors appear in the JS console

Offline Steps

No changes are expected, offline elements should be greyed out; errors on relaunch should be displayed as before.

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

All screenshots are available here: https://www.dropbox.com/s/1nv9cicwl758ea7/Screenshots.zip?dl=0

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

shawnborton · 2022-11-03T13:59:51Z

src/styles/styles.js

@@ -2761,6 +2761,20 @@ const styles = {
        fontSize: variables.fontSizeSmall,
        ...spacing.ph2,
    },
+
+    workspaceSection: {
+        backgroundColor: colors.gray1,


I wonder if we can make a more global value here that comes from the theme file... something like themeColors.cardBackground or something? Then in the theme file, use gray1

Yes, ideally we don't want to import colors anywhere except into the theme file. I'll be working on this when I update the colors in the upcoming week, but feel free to add as a variable todefault.js for now!

Sounds good!

shawnborton · 2022-11-03T14:00:28Z

src/styles/styles.js

@@ -2761,6 +2761,20 @@ const styles = {
        fontSize: variables.fontSizeSmall,
        ...spacing.ph2,
    },
+
+    workspaceSection: {


I think this is fine for now, but I could see making this even more generic... like cardSection or something? In case we use it elsewhere

yeah I like that too. This was already more genericized than my first attempt (workspaceEditorSection) but will make it even broader.

shawnborton · 2022-11-03T14:01:02Z

src/styles/styles.js

+    workspaceSection: {
+        backgroundColor: colors.gray1,
+        borderRadius: variables.componentBorderRadiusCard,
+        marginVertical: 10,


Maybe we can just do bottom margin for all of these cards? This way the first card in a view will sit more towards the top.

oh that works

grgia · 2022-11-08T18:27:23Z

src/pages/workspace/reimburse/WorkspaceReimburseView.js

                        menuItems={[
                            {
                                title: this.props.translate('workspace.reimburse.viewAllReceipts'),
                                onPress: () => Link.openOldDotLink(`expenses?policyIDList=${this.props.policy.id}&billableReimbursable=reimbursable&submitterEmail=%2B%2B`),
                                icon: Expensicons.Receipt,
                                shouldShowRightIcon: true,
                                iconRight: Expensicons.NewWindow,
+                                iconFill: themeColors.buttonSuccessBG,


if we're holding for new color theme, you'll want to use themeColors.success

You can wait on this until the colors are merged, I just wanted to make a note

Cool! Will keep this in here for now for testing purposes but can swap it once that's ready to go.

dangrous · 2022-11-08T23:22:12Z

Updated! I think I caught everything in the Figma you shared. There are a couple more places in settings where we use the same Section component, which I think are also the places we might want to update illustrations. They are:

Enable Step (I think this is in the bank account setup flow but I'm not sure)
Validation Step (I think this is in the bank account setup flow but I'm not sure)
Workspace Bank Account Page
Workspace Bills VBA View
Workspace Card No VBA View
Workspace Card VBA No ECard View
Workspace Card VBA With ECard View
Workspace Invoices VBA View
Workspace Reimburse View (there's a fourth card for if they have a VBA)
Workspace Travel VBA View

I think all of those will show up on an account that has a bank account connected - except for Workspace Card No VBA View which you may be able to get to now. I think the Card is referencing the Expensify card? I will try to get some screenshots of the existing design of the others so you can work on what you want instead!

There are also a couple places we use Sections that AREN'T settings related. I think it still would likely be worth changing these as well, but would love to know what you think. If we do update them, I'll go through this code and put all the styles into the component as defaults instead of passing it each time here. Those places are:

Get Assistance Page
Request Call Page

Also, I didn't update the button shapes (since that will be global, right?) or the icons (since I don't have the icons yet) but happy to do either/both if we have the goods.

And, let me know if you want me to rename any of the illustrations; these should mostly be fine but some are a little wonky (if we're eventually going to remove the existing ones we can be a little more broad in how we name these).

Lastly, there are some merge conflicts, I'll work on those separately.

shawnborton · 2022-11-08T23:32:49Z

Yup, no worries on the button shape or icons just yet.

For those sections you referenced, can you share screenshots of them here?

dangrous · 2022-11-09T19:47:57Z

@shawnborton here you go! There are a couple I couldn't quite figure out how to get at yet, so I screenshotted the code (that notes what icons are used). I realize that's not super helpful but figured you could start on the others and then I will keep figuring out how to see the remaining four.

Remaining Sections.zip

Also please remind me that in this PR i changed the background color of the sections to blue so I could see them better, so I need to update that before we go much further.

shawnborton · 2022-11-09T23:06:26Z

Here's another batch of illustrations to use: MoreSimpleIllustrations.zip

I tried going through your screenshots in the same Figma file... here are some proposed ideas:

We might want to change the headline for this one, the equals sign made the break a bit awkward:

dangrous · 2022-11-10T22:26:33Z

Awesome, thanks @shawnborton ! I've updated all of the screens and so I'm going to go through and screenshot their final forms (and really try to get those last few). Now that we have updated ALL of them, I'm going to switch out the styling changes from each individual instance into the main <Section> component, too. Will post screenshots here, and then I think we just hold for the new colors/icons!

shawnborton · 2022-11-15T00:26:44Z

Sounds great, thanks for the update! I'm planning to do a weekly update for the overall branding tasks in the open source room tomorrow, so let me know where we're at by tomorrow morning when you get a moment.

dangrous · 2022-11-15T15:27:01Z

Haven't had time for anything yet but things are slowing down outside of this PR so I plan to have it completed and ready for final review later today!

shawnborton · 2022-11-15T20:15:37Z

Sounds great, thanks for the update!

dangrous · 2022-11-15T23:16:25Z

Ok have been thrown in a bunch of different directions today even though I was hoping not to be. Started going through this and will be able to finish tomorrow.

dangrous · 2022-11-16T20:10:12Z

Just committing some more updates here. Just the last four remaining (the harder ones to figure out where they actually exist... Can track progress here: https://docs.google.com/spreadsheets/d/1NEe1pc-1Wy2f0gpSCPPAb0SsHSAeXub56BuX_6-oaBU/edit#gid=0

dangrous · 2022-11-16T21:09:30Z

Updated and ready for review! I believe this should remain on hold until the new colors/icons are ready because I'll likely have to make some final changes at that point. But, it's all synced up with the new style in all the sections.

Screenshots, labelled by the number noted in the Google Sheet are here - all 138 of them. There are also a couple marked "OLD" which is for places I was able to get what they looked like before.

Lastly, I didn't make that copy change (the = one) because I figured we should wait for the new fonts in case the line break was different. I believe I made all the other changes, but take a look at the screenshots to make sure - you only really have to look at one of the platforms, they're pretty much all the same.

aldo-expensify · 2022-11-22T01:29:10Z

Thoughts @shawnborton @grgia ?

shawnborton · 2022-11-22T01:33:38Z

I am down to merge and do follow ups for the small things we might have missed. Thanks so much for the review Aldo, I know this was a big one!

aldo-expensify · 2022-11-22T02:20:05Z

@grgia do you want to review before I merge?

grgia

LGTM, tested dark mode

shawnborton · 2022-11-22T19:54:20Z

@Santhosh-Sellavel all you!

Santhosh-Sellavel · 2022-11-22T23:39:23Z

Thanks for the bump on it already!

Santhosh-Sellavel · 2022-11-23T01:23:57Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Note missed a few screens that I can't access like with an Expensify card.

Web

Screen.Recording.2022-11-23.at.6.21.52.AM.mov

Mobile Web - Chrome

Screen_Recording_20221123_063938_Chrome.mp4

Mobile Web - Safari

RPReplay_Final1669165540.MP4

Desktop

Screen.Recording.2022-11-23.at.6.29.38.AM.mov

iOS & Android

Screen.Recording.2022-11-23.at.6.41.33.AM.mov

Santhosh-Sellavel · 2022-11-23T01:26:37Z

Is this expected on offline, shouldn't buttons be disabled?
cc: @shawnborton

shawnborton · 2022-11-23T01:28:28Z

Hmm good catch, how does it currently work on the main branch?

Santhosh-Sellavel · 2022-11-23T01:30:28Z

Same behaviour on main too!

shawnborton · 2022-11-23T01:32:30Z

Okay cool, then no worries if it's the same thing on main.

Santhosh-Sellavel · 2022-11-23T01:37:08Z

Should we log a bug for it @shawnborton ?

Santhosh-Sellavel

Looks good!

shawnborton · 2022-11-23T01:51:09Z

@Santhosh-Sellavel can't hurt, you can bring that up in #expensify-bugs

grgia · 2022-11-23T18:21:54Z

If the bug is logged, am I good to merge this?

aldo-expensify · 2022-11-23T18:46:49Z

@shawnborton from this slack conversation , I understand that we should proceed with the merge, right?

aldo-expensify · 2022-11-23T18:52:56Z

Lol, now I see that Luke posted "Buttons merged", so I guess that was about a different PR

shawnborton · 2022-11-23T18:53:07Z

Yup, let’s do it!

…

On Wed, Nov 23, 2022 at 10:47 AM Aldo Canepa Garay ***@***.***> wrote: @shawnborton <https://github.com/shawnborton> from this slack <https://expensify.slack.com/archives/C01GTK53T8Q/p1669147855166289> conversation , I understand that we should proceed with the merge, right? — Reply to this email directly, view it on GitHub <#12419 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWH5R3RLD2IVAOVC3UDLLWJZRCHANCNFSM6AAAAAARVUSZKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

aldo-expensify · 2022-11-23T18:53:31Z

Done!

OSBotify · 2022-11-23T18:57:29Z

✋ 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 · 2022-11-24T00:51:37Z

🚀 Deployed to staging by @aldo-expensify in version: 1.2.31-0 🚀

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

luacmartins · 2022-12-06T17:28:26Z

The production deploy comment failed for this PR, but this was deployed to production on v1.2.32-2 on Nov 28.

dangrous · 2022-12-13T22:57:42Z

As requested in #13318 (comment) - commenting here. This was not really a bug per se so we don't need a new process. Generally I guess we should make sure that if multiple PRs are affecting something (say, a redesign), we make sure that all of them are considered in each. In this case, we were matching the style of the old buttons, and not the new ones.

initial updates for style on workspace editor page

f8dc72e

dangrous self-assigned this Nov 3, 2022

dangrous changed the title ~~initial updates for style on workspace editor page~~ Workspace Editor Page Design Updates Nov 3, 2022

shawnborton reviewed Nov 3, 2022

View reviewed changes

grgia reviewed Nov 8, 2022

View reviewed changes

update pages and consolidate images

09ed8b1

dangrous added 2 commits November 9, 2022 15:23

prep to fix merge conflicts

ddb7718

merge main and handle conflicts

f775b54

dangrous added 2 commits November 15, 2022 19:20

add confirmed 50% of the new cards

afd5a33

add all but 4 updated cards

a681a68

final components

b6e335f

dangrous changed the title ~~Workspace Editor Page Design Updates~~ [HOLD] Workspace Editor Page Design Updates Nov 16, 2022

dangrous marked this pull request as ready for review November 16, 2022 21:42

dangrous requested a review from a team as a code owner November 16, 2022 21:42

melvin-bot bot requested review from aldo-expensify and Santhosh-Sellavel and removed request for a team November 16, 2022 21:42

aldo-expensify approved these changes Nov 22, 2022

View reviewed changes

grgia approved these changes Nov 22, 2022

View reviewed changes

shawnborton approved these changes Nov 22, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Nov 23, 2022

View reviewed changes

aldo-expensify merged commit d849d85 into main Nov 23, 2022

aldo-expensify deleted the dangrous-workspace-editor branch November 23, 2022 18:53

OSBotify mentioned this pull request Nov 23, 2022

Deploy Checklist: New Expensify 2022-11-23 #12976

Closed

38 tasks

Santhosh-Sellavel mentioned this pull request Dec 5, 2022

[$1000] [Hold for payment 2022-12-06] Update workspace editor illustrations #12257

Closed

dangrous mentioned this pull request Dec 13, 2022

[HOLD for payment 2022-12-20] Connect bank account - The green option row and standard option row don’t line up correctly & icons and text are not aligned #13318

Closed

Workspace Editor Page Design Updates #12419

Workspace Editor Page Design Updates #12419

Conversation

dangrous commented Nov 3, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Offline Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

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

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

dangrous commented Nov 8, 2022

shawnborton commented Nov 8, 2022

dangrous commented Nov 9, 2022

shawnborton commented Nov 9, 2022

dangrous commented Nov 10, 2022

shawnborton commented Nov 15, 2022

dangrous commented Nov 15, 2022

shawnborton commented Nov 15, 2022

dangrous commented Nov 15, 2022

dangrous commented Nov 16, 2022

dangrous commented Nov 16, 2022

aldo-expensify commented Nov 22, 2022

shawnborton commented Nov 22, 2022

aldo-expensify commented Nov 22, 2022

grgia left a comment

Choose a reason for hiding this comment

shawnborton commented Nov 22, 2022

Santhosh-Sellavel commented Nov 22, 2022

Santhosh-Sellavel commented Nov 23, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

Santhosh-Sellavel commented Nov 23, 2022

shawnborton commented Nov 23, 2022

Santhosh-Sellavel commented Nov 23, 2022

shawnborton commented Nov 23, 2022

Santhosh-Sellavel commented Nov 23, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

shawnborton commented Nov 23, 2022

grgia commented Nov 23, 2022

aldo-expensify commented Nov 23, 2022

aldo-expensify commented Nov 23, 2022

shawnborton commented Nov 23, 2022 via email

aldo-expensify commented Nov 23, 2022

OSBotify commented Nov 23, 2022

OSBotify commented Nov 24, 2022

luacmartins commented Dec 6, 2022

dangrous commented Dec 13, 2022

dangrous commented Nov 3, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 23, 2022 •

edited

Loading