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

[HOLD for payment 2023-04-20] [$1000] General settings - Default currency - Background is lighter than the main one #15460

Closed
5 of 6 tasks
kbecciv opened this issue Feb 23, 2023 · 80 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Feb 23, 2023

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 URL https://staging.new.expensify.com/
    or open the App
  2. Login with any account
  3. Navigate to Settings > any WS or create a new Workspace
  4. Tap General settings > Default currency

Expected Result:

The background color should be dark greenish like the main one.

Actual Result:

The background is light gray in Android App and white in mWeb/Chrome and mWeb/Safari

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.2.76.3

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5951355_Screen_Recording_20230224_001353_New_Expensify.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d092765565836aa7
  • Upwork Job ID: 1629091498970095616
  • Last Price Increase: 2023-03-20
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 23, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 23, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Feb 23, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@flaviadefaria
Copy link
Contributor

I can't reproduce the bug on IOS but can reproduce it on other platforms - adding the external label.

@flaviadefaria flaviadefaria added the External Added to denote the issue can be worked on by a contributor label Feb 24, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 24, 2023
@melvin-bot melvin-bot bot changed the title General settings - Default currency - Background is lighter than the main one [$1000] General settings - Default currency - Background is lighter than the main one Feb 24, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~01d092765565836aa7

@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 24, 2023
@MelvinBot
Copy link

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

@PankajAS
Copy link
Contributor

PankajAS commented Feb 24, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Default currency dropdown background color is not same as primary color of app

What is the root cause of that problem?

Android is using native style picker that's why its showing default grey color of picker

What changes do you think we should make in order to solve the problem?

For solution we need to create custom drop-down component which style will be same for all platform because react-native-picker-select library using react-native-picker/picker internally and its has different native style for all platform , here what we will follow in code:

  1. Create new component with name PickerSelect
  2. Use Modal to show options to choose.
  3. Use Text and Arrow icon to show selected option.

What alternative solutions did you explore? (Optional)

None

@redstar504
Copy link
Contributor

redstar504 commented Feb 24, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Picker menus do not apply a consistent color scheme across all platforms.

What is the root cause of that problem?

Each platform provides it's own underlying system element which is used for rendering the Picker component.

For web and iOS native, the Expensify green color is properly applied for the background, but for mobile web, default system components are used that we have no control over. These mobile web components feel natural and do not conflict with the app styling so they don't seem to be pose a problem.

The problem is that the Android Native picker, which provides more of a built-in feel, displays with a gray background color that is largely out of character with the company styling. The native picker extends a default system theme which supplies it's background color. This is something we do have control over, and therefore I propose we address this as our solution.

What changes do you think we should make in order to solve the problem?

Android provides a way to change the appearance of system elements by using properties defined in XML format. In a React Native app, these styles can be applied by editing the styles.xml file located in the /android directory. For instance, if we want to change the background color of the Picker, we could apply the colorBackgroundFloating property.

To achieve more detailed customizations like changing the font family, adding a border, or applying a corner radius, we can define a new popupTheme that uses a drawable. This approach can also be applied to customize the styling of other elements too, such as Date and Alert dialogs. By defining custom themes through XML, we can achieve a more customized look for these UI elements that more closely matches our branding.

Example 1 (using colorBackgroundFloating)

Screenshot from 2023-02-24 13-04-23

Example 2 (using a PopupTheme)

Background color, corner radius, and Expensify Neue font

2023-03-06.05-29-01.mp4
Example 3 (using a PopupTheme -- recommended style by @shawnborton)

Background color, corner radius, border, and Expensify Neue font

2023-03-07.02-08-31.mp4

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Feb 27, 2023
@sobitneupane
Copy link
Contributor

@redstar504

We can override styles on Android native directly by applying xml in the /android directory.

Can you please explain a bit more about how you are going to do that? Please make use of permalink to explain where changes will be made.

@melvin-bot melvin-bot bot removed the Overdue label Feb 27, 2023
@PankajAS
Copy link
Contributor

@sobitneupane changes which purposed by @redstar504 only for android native but other platform will still use their native style picker.

@sobitneupane
Copy link
Contributor

@PankajAS Thanks. I thought the issue is existent only on Android.

@sobitneupane
Copy link
Contributor

sobitneupane commented Feb 27, 2023

We first need to list all the components where we are having similar issue. And If we have to create a custom component, I think we should consult with design team.

cc: @chiragsalian

@sobitneupane
Copy link
Contributor

Components Using Picker:

  • AddPlaidBankAccount
  • CountryPicker
  • LocalePicker
  • StatePicker
  • ReportSettingsPage
  • CompanyStep
  • WorkspaceNewRoomPage
  • WorkspaceSettingsPage
  • WorkspaceReimburseView

@PankajAS
Copy link
Contributor

We first need to list all the components where we are having similar issue. And If we have to create a custom component, I think we should consult with design team.

cc: @chiragsalian

Yes @sobitneupane All platform should have same color and style of picker, we cant change for only one platform that's why we need custom component and we can consult with design team

@redstar504
Copy link
Contributor

redstar504 commented Feb 28, 2023

While building a picker that looks identical on each platform is a respectable undertaking, I think we should also consider the expectations of users who are accustomed to the specific behaviors of forms on each platform.

Regarding the gray background on Android, it appears obvious that it is out of character with the rest of the styling. In order to fix that, I suggest using XML styles to achieve the green background and white text appearance. Custom styles can be added here.

@melvin-bot melvin-bot bot added the Overdue label Mar 1, 2023
@flaviadefaria
Copy link
Contributor

What's the latest here?

@flaviadefaria flaviadefaria added the Bug Something is broken. Auto assigns a BugZero manager. label Apr 6, 2023
@MelvinBot
Copy link

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

@Expensify Expensify deleted a comment from MelvinBot Apr 6, 2023
@sophiepintoraetz
Copy link
Contributor

Yup - let's change the date (I almost went to pay) @chiragsalian, do you know when that would be?

@chiragsalian
Copy link
Contributor

do you know when that would be?

I believe it will be set to 7 days after the code is live. Currently the fixes are on staging and its not live yet. I'll remove the payment title and remove the payment label for now. They should automatically get set once the PR is live.

@chiragsalian chiragsalian changed the title [HOLD for payment 2023-04-03] [$1000] General settings - Default currency - Background is lighter than the main one [$1000] General settings - Default currency - Background is lighter than the main one Apr 11, 2023
@chiragsalian chiragsalian removed the Awaiting Payment Auto-added when associated PR is deployed to production label Apr 11, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Apr 13, 2023
@melvin-bot melvin-bot bot changed the title [$1000] General settings - Default currency - Background is lighter than the main one [HOLD for payment 2023-04-20] [$1000] General settings - Default currency - Background is lighter than the main one Apr 13, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 13, 2023
@MelvinBot
Copy link

Reviewing label has been removed, please complete the "BugZero Checklist".

@MelvinBot
Copy link

MelvinBot commented Apr 13, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.99-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-04-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@MelvinBot
Copy link

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@sobitneupane / @chiragsalian] The PR that introduced the bug has been identified. Link to the PR:
  • [@sobitneupane / @chiragsalian] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@sobitneupane / @chiragsalian] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@sophiepintoraetz] Determine if we should create a regression test for this bug.
  • [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@sophiepintoraetz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@redstar504
Copy link
Contributor

redstar504 commented Apr 18, 2023

Looks like this should be good to pay out tomorrow? It will be 7 days since the regression PR was deployed.

Also wanted to mention that we forgot to update the Upwork price to $2000 per our agreement here #15460 (comment)

It would have been eligible for the bonus in addition to that, but due to the regression the bonus should not be applied.

cc @sophiepintoraetz @chiragsalian

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 19, 2023
@sophiepintoraetz
Copy link
Contributor

So just to summarise, the payments here are:

@melvin-bot melvin-bot bot added the Overdue label Apr 24, 2023
@redstar504
Copy link
Contributor

@sophiepintoraetz Just wanted to bump this regarding payment. @sobitneupane reacted to your comment but you probably didn't get notified. The regression was fixed and merged in #17063.

@melvin-bot melvin-bot bot removed the Overdue label Apr 25, 2023
@sobitneupane
Copy link
Contributor

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • The PR that introduced the bug has been identified. Link to the PR:

Rather than a bug, it is an improvement on the app.

cc: @chiragsalian

@sobitneupane
Copy link
Contributor

Regression Test Proposal

  1. Launch the Android native app
  2. Log in and navigate to a screen containing a picker and open the picker(Workspace > General Settings > Default currency)
  3. Navigate to a screen containing a DatePicker and open the date picker(Workspace > Connect Bank Account > Connect Manually > Step 2 > Incorporation Date)
  4. Navigate to a screen that triggers an alert (Profile > Upload Profile Image > Deny Camera Access)
  5. Confirm that the picker, date picker and alert has a green background.
  6. Confirm the picker, date picker and alert has a border radius.
  7. Confirm the items are displayed with a white text color, in the Expensify Neue font.

Do we agree 👍 or 👎

@sobitneupane
Copy link
Contributor

can we confirmed this has been resolved?

@sophiepintoraetz Yep. This issue has been resolved.

@sophiepintoraetz
Copy link
Contributor

@redstar504 - I have been OOO for the past few days but the offers should be in your inbox now.
https://www.upwork.com/jobs/~01bbbc2fad2f19d1bf

@sophiepintoraetz
Copy link
Contributor

We're all set here.

Copy link

melvin-bot bot commented Dec 15, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests