Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.43k stars 2.8k forks source link

[Hold for payment 2023-1-31][$4000] Onfido - Countries in the list are not visible #13262

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to any workspace and attempt the Connect Bank Account step
  2. Complete All Steps Until Onfido flow triggers in the personal information page
  3. Click on the issuing country dropdown

Expected Result:

Country Names Should Be Visible

Actual Result:

Country Names Are Not Visible

Workaround:

unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.35-0 Reproducible in staging?: y Reproducible in production?: Can't check Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/205200240-8da01228-09f5-4b92-8c89-b53c67e42908.mov

2

Expensify/Expensify Issue URL: Issue reported by: @syedsaroshfarrukhdot Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1669903794406069

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011647945c75686014
  • Upwork Job ID: 1598766183105921024
  • Last Price Increase: 2022-12-22
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

syedsaroshfarrukhdot commented 1 year ago

Proposal

We can simply override .onfido-sdk-ui-CountrySelector-CountryDropdown-custom__menu--overlay with desired color as we are currently overriding Onfido styles also to make it look nice

.onfido-sdk-ui-CountrySelector-CountryDropdown-custom__menu--overlay{
    background: #061B09 !important;
}

https://github.com/Expensify/App/blob/39b2f9924060c7381b2ff37563a5af88360b0d9d/src/components/Onfido/index.css#L1-L13

Also As Discussed In Slack Conversation The Button After Selecting Styles Are Also Missing Styling For DarkMode So We Can Also Fix It in This GH Issue As The Flow is Same

https://expensify.slack.com/archives/C049HHMV9SM/p1669904053931079

We need to pass colorBackgroundDocTypeButton : themeColors.success, and change colorBackgroundIcon to colorBackgroundIcon: themeColors.success, in customUI object in BaseOnfidoWeb.js

After this dropdown and button styling along with the dropdown will be fixed

After Fix :-

https://user-images.githubusercontent.com/81307212/205205022-bc3a82d6-df3c-4faa-ba4f-5039bf79c5fc.mov

MitchExpensify commented 1 year ago

Replicated. Looks unique and noticed due to recent Dark Mode switch:

image

Labeling external

melvin-bot[bot] commented 1 year ago

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~011647945c75686014

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @tylerkaraszewski (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Pujan92 commented 1 year ago

Proposal

  1. Change the background color of the custom menu class of onfido

    .onfido-sdk-ui-CountrySelector-CountryDropdown-custom__menu {
    background-color: #061B09 !important;
    }
  2. Doctype button background change with the icon background

    colorBackgroundDocTypeButton: themeColors.success,
    colorBackgroundIcon: themeColors.transparent,
  3. Button properties change(remove box shadow on hover,focus) to make it like the expensify button

    .onfido-sdk-ui-DocumentSelector-DocumentList-option.onfido-sdk-ui-DocumentSelector-DocumentList-optionHoverDesktop:hover {
    box-shadow: none !important;
    background-color: #00C271 !important;
    }
    .ods-button.-action--primary:focus {
    box-shadow: none !important;
    }

a2f2c81c-7e6e-4660-8ab5-d4b11c8ea0e3.webm

syedsaroshfarrukhdot commented 1 year ago

Update Proposal :-

Issue with above proposals https://github.com/Expensify/App/issues/13262#issuecomment-1336141513 and My Proposal https://github.com/Expensify/App/issues/13262#issuecomment-1334700583 is as we are hardcoding the color so when when we will implement dark mode switch list will appear like this in the light mode.

Screen Shot 2022-12-03 at 5 26 17 PM

We need to set appBG color to fix this regression which will be caused in future. We need to set appBG color like this to the list background in componentDidMount inBaseOnfidoWeb.js

    const root = document.documentElement;
    root.style.setProperty('--listcolor' , themeColors.appBG)

and in index.css we can use

   .onfido-sdk-ui-CountrySelector-CountryDropdown-custom__menu--overlay{
         background: var(--listcolor) !important;
    }

Also As Discussed In Slack Conversation The Button After Selecting Styles Are Also Missing Styling For DarkMode So We Can Also Fix It in This GH Issue As The Flow is Same

https://expensify.slack.com/archives/C049HHMV9SM/p1669904053931079

We need to pass colorBackgroundDocTypeButton : themeColors.success, and change colorBackgroundIcon to colorBackgroundIcon: themeColors.transparent, in customUI object in BaseOnfidoWeb.js

if we want to make it exactly like our other buttons we can remove box shadows on the button by setting these classes in index.css also

  .onfido-sdk-ui-DocumentSelector-DocumentList-option.onfido-sdk-ui-DocumentSelector-DocumentList-optionHoverDesktop:hover {
        box-shadow: none !important;
            background-color: #00C271 !important;
    }
   .ods-button.-action--primary:focus {
           box-shadow: none !important;
    }

And to fix button text color all along on both modes we can also set in componentDidMount inBaseOnfidoWeb.js

  root.style.setProperty('--textcolor' , themeColors.textLight) 

And in index.css we can set text color like this

  .onfido-sdk-ui-DocumentSelector-DocumentList-label{
        color: var(--textcolor) !important;
    }
   .onfido-sdk-ui-DocumentSelector-DocumentList-hint{
      color:  var(--textcolor)  !important;
    }

After Fixing In DarkMode :-

https://user-images.githubusercontent.com/81307212/205441438-9a2f85f3-a9fd-433a-b646-c9312d2ec8cf.mov

After Fixing In Light Mode :-

https://user-images.githubusercontent.com/81307212/205441973-639f0e83-12a4-4d04-afc1-9f29cbee6d98.mov

parasharrajat commented 1 year ago

I checked the documentation https://github.com/onfido/onfido-sdk-ui/blob/HEAD/UI_CUSTOMIZATION.md but it does not have any details on customizing the dropdowns. I can not find the source as well.

With this info, I am fine with @syedsaroshfarrukhdot 's proposal.

Things to improve:

  1. Use more specific CSS variable names.

cc: @tylerkaraszewski

:ribbon: :eyes: :ribbon: C+ reviewed


It seems we already have design approval on Slack.

syedsaroshfarrukhdot commented 1 year ago

@parasharrajat Thanks, for reviewing sure will keep that in mind and will use more specific CSS variable names while creating PR.

MitchExpensify commented 1 year ago

Hired @syedsaroshfarrukhdot on the Upwork job (The correct price will be reflected when payment is made)

Also hired @parasharrajat!

syedsaroshfarrukhdot commented 1 year ago

@MitchExpensify Thanks for hiring should I be making a PR ? As I haven't been assigned to job here on GH till now.

MitchExpensify commented 1 year ago

I think hold on being assigned the job by @tylerkaraszewski. You reported the bug so needed payment either way regardless of whether thats payment just for reporting or payment for reporting and contribution

syedsaroshfarrukhdot commented 1 year ago

Oh my bad. Sure will wait for being assigned to the job by @tylerkaraszewski.

MitchExpensify commented 1 year ago

All good!

marcaaron commented 1 year ago

I checked the documentation https://github.com/onfido/onfido-sdk-ui/blob/HEAD/UI_CUSTOMIZATION.md but it does not have any details on customizing the dropdowns. I can not find the source as well.

We can ask them if they can allow it to be customized. The solutions provided are less than ideal imo.

marcaaron commented 1 year ago

also I believe we are going to replace the entire Onfido SDK with a custom implementation.

But not sure when that is happening exactly.

syedsaroshfarrukhdot commented 1 year ago

We can ask them if they can allow it to be customized. The solutions provided are less than ideal imo.

Yes, We can ask them to do so or raise a PR against their repo to support dropdown and button customization ideally it would be the best solution but it would take a lot of time to do so. Due to this issue Onfido SDK is currently unusable as lists doesn't show in our current dark theme. In my opinion we can overwrite these styles for now as we are also doing it already. Along with it we can began the process with them to support dropdown and button customization.

https://github.com/Expensify/App/blob/39b2f9924060c7381b2ff37563a5af88360b0d9d/src/components/Onfido/index.css#L1-L13

marcaaron commented 1 year ago

ideally it would be the best solution but it would take a lot of time to do so

Maybe it will. Did we ask to be sure?

Due to this issue Onfido SDK is currently unusable as lists doesn't show in our current dark theme.

That's a good point. Reverting dark mode doesn't feel like a realistic option.

In my opinion we can overwrite these styles for now as we are also doing it already.

We introduced a hack already so we should be allowed to add even more hacks!

I think this idea exhibits why we should avoid hacks.

parasharrajat commented 1 year ago

We can ask them if they can allow it to be customized. The solutions provided are less than ideal imo.

Yeah, if that can be done. It would be great.

@syedsaroshfarrukhdot Do you think you can raise an issue on https://github.com/onfido/onfido-sdk-ui for this?

syedsaroshfarrukhdot commented 1 year ago

Sure, I will raise the issue with them and provide an update accordingly.

MitchExpensify commented 1 year ago

I'm unsure of the status of this issue, are we moving forward with any proposal here, waiting on a proposal we agree on, or will it be solved when we replace the entire Onfido SDK with a custom implementation?

parasharrajat commented 1 year ago

We are waiting for the updates on the upstream repo.

parasharrajat commented 1 year ago

It seems the SDK also uses CSS https://github.com/onfido/onfido-sdk-ui/blob/adb6b84a4431f885ee7a152227412b7f7335dece/src/components/CountrySelector/CountryDropdown/style.scss so CSS must be the ultimate solution but let's wait a few days to see if maintainers can help.

MitchExpensify commented 1 year ago

I'm heading ooo so need to assign a new CM - Reapplying the External label for auto assignment.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

Current assignee @parasharrajat is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Current assignee @tylerkaraszewski is eligible for the External assigner, not assigning anyone new.

marcaaron commented 1 year ago

Thanks @syedsaroshfarrukhdot I sent a message to Onfido to try and expedite a response.

tylerkaraszewski commented 1 year ago

If anyone wants to take this, they can, but it looks like it's blocked on Onfido currently?

syedsaroshfarrukhdot commented 1 year ago

@tylerkaraszewski Yes, we are waiting for a response from Onfido to support it natively. Otherwise with css which would be the ultimate solution if we don't get a response my Proposal tests out well and have been reviewed by C+ also. I have created an GH Issue with them which can be tracked.

syedsaroshfarrukhdot commented 1 year ago

Just for the reference I found an other issue on there upstream repo where Onfido Team recommended to overwrite there CSS until customization are released in the next update.

puneetlath commented 1 year ago

Interesting find @syedsaroshfarrukhdot. However, that comment seems to indicate that they added support in version 8.2.0 and I see that they are now on version 10.1.0. So could we solve this by simply updating to the latest version?

syedsaroshfarrukhdot commented 1 year ago

Interesting find @syedsaroshfarrukhdot. However, that comment seems to indicate that they added support in version 8.2.0 and I see that they are now on version 10.1.0. So could we solve this by simply updating to the latest version?

@puneetlath No, I don't think we can solve it by updating to latest version. As they currently doesn't support customization for the dropdown according to there docs for the latest version and the Comment by Onfido team on the issue I created they recognize it as a bug and will be discussing it with there design to resolve these issues.

I just pointed out that as onfido team also suggested to override there CSS styles while having such issue in the past. We could do similarly to escalate this process for now as in My Proposal as we are currently in #BugZero I guess.

I will keep following the Issue once they release the updated version with dropdown and doctype button text colors we can use those props provided by them and remove CSS customizations.

marcaaron commented 1 year ago

Nice, that's great to hear they were responsive!

tylerkaraszewski commented 1 year ago

Still waiting on onfido it seems.

JmillsExpensify commented 1 year ago

This issue landed on my radar given that it's close to hitting 30 days. Given that a fork isn't in play, and related, we will eventually deprecate the Onfido SDK in the coming months (best ETA we can give, though nothing is certain), then I think we should update the title and HOLD this issue on Onfido.

melvin-bot[bot] commented 1 year ago

@tylerkaraszewski, @puneetlath, @parasharrajat Eep! 4 days overdue now. Issues have feelings too...

syedsaroshfarrukhdot commented 1 year ago

We have an update from Onfido Team they will adding support for the both options (country selection drop-down background color and document type button background color on hover and active states). They will allow these customizations in next planned release. I have asked them about ETA of next release soon we will have an update regarding it.

parasharrajat commented 1 year ago

Awesome. Thanks for the update.

marcaaron commented 1 year ago

Oh Nice! 🎉

syedsaroshfarrukhdot commented 1 year ago

We have an update on ETA of the new Onfido Release from the Onfido Team with support for the both options (country selection drop-down background color and document type button background color on hover and active states). It should be released by 16th Jan.

parasharrajat commented 1 year ago

Awesome. Thanks for the update. Please submit the updated proposal when the new version is released and we can get this going.

syedsaroshfarrukhdot commented 1 year ago

Sure will do that 🙂

syedsaroshfarrukhdot commented 1 year ago

Updated Proposal :-

Planned Update From Onfido On 16 Jan Was Release Earlier Today With The Support (country selection drop-down background color and document type button background color on hover and active states along with border colors).

So now we need to change version of onfido-sdk-ui": "^8.1.0 to onfido-sdk-ui": "^10.3.0 in package.json and run npm install command.

We need to add following props to fix country dropdown background color and DocTypeButton colors in BaseOnfidoWeb.js

diff --git a/src/components/Onfido/BaseOnfidoWeb.js b/src/components/Onfido/BaseOnfidoWeb.js
index b01b736ff..366e98c7c 100644
--- a/src/components/Onfido/BaseOnfidoWeb.js
+++ b/src/components/Onfido/BaseOnfidoWeb.js
@@ -53,6 +53,11 @@ class Onfido extends React.Component {
                 colorBackgroundLinkActive: themeColors.link,
                 authAccentColor: themeColors.link,
                 colorBackgroundInfoPill: themeColors.link,
+                colorBackgroundSelector: themeColors.appBG,
+                colorBackgroundDocTypeButton : themeColors.success,
+                colorBackgroundDocTypeButtonHover : themeColors.successHover,
+                colorBackgroundIcon: themeColors.transparent,
+                colorBorderDocTypeButtonHover : themeColors.transparent,
             },

After Fixing :-

https://user-images.githubusercontent.com/81307212/211853420-296b97bc-81f9-4282-9920-367acf8f4711.mov

Just Noticed Another Issue With Get Secure Link Button border hover color is different so we can also fix it in this scope of the issue.

https://user-images.githubusercontent.com/81307212/211857938-92b4757f-1f7c-488d-9e2f-e5b293bb1236.mov

By Using Following Props

   colorBorderButtonPrimaryHover : themeColors.transparent,

After Fixing :-

https://user-images.githubusercontent.com/81307212/211858256-1e449fac-8662-416c-8585-f31ec84b81e6.mov

CC :- @parasharrajat

JmillsExpensify commented 1 year ago

Wow really great work pushing this forward! 🙌🏼

parasharrajat commented 1 year ago

Sorry for the delay here, the comment was never submitted.

@JmillsExpensify Please remove the HOLD from the title.

@syedsaroshfarrukhdot 's updated proposal looks good to me. You will have to make sure that nothing breaks due to the upgrade.

we don't have to update the border focus colors in this PR and also we are not changing that in the app at this stage.

cc: @tylerkaraszewski

:ribbon: :eyes: :ribbon: C+ reviewed

syedsaroshfarrukhdot commented 1 year ago

Thanks for reviewing. Will definitely thoroughly test and will make sure that it doesn't break anything after upgrade. Noted will not update buttons border focus colors in this PR.

syedsaroshfarrukhdot commented 1 year ago

Will I was working on the PR I noticed a problem with onfido-sdk-ui on android/Chrome. It gives Unsupported Browser error on the android/Chrome while even chrome is updated to latest version. Which makes testing it on android/Chrome not possible.

This issue exists on bothonfido-sdk-ui": "^8.1.0 and the latest onfido-sdk-ui": "^10.3.0

So I believe we should create another GH Issue and open a new upstream issue with onfido to get it resolved.

WhatsApp Image 2023-01-14 at 6 33 44 PM

Should I move forward with the PR ignoring mWeb/Chrome as it isn't messed up due to the upgrade it existed before ?

Other Platforms It Is Working Completely Fine Can Check Below.

Desktop
Web
IOS
Android
mWeb/Safari

CC : @parasharrajat @tylerkaraszewski