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.32k stars 2.76k forks source link

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

Closed kbecciv closed 1 year ago

kbecciv 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 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?

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

https://user-images.githubusercontent.com/93399543/221048733-97633b07-565e-4f1f-a7d2-48d5e1ed605d.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
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

flaviadefaria commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

PankajAS commented 1 year ago

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 commented 1 year ago

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](https://user-images.githubusercontent.com/1311325/221291668-7401a49b-9994-48e6-ac17-df2678d97ace.png)
Example 2 (using a PopupTheme) #### Background color, corner radius, and Expensify Neue font https://user-images.githubusercontent.com/1311325/223130116-1c6d70df-2207-47f7-90fa-7c5cb3540f70.mp4
Example 3 (using a PopupTheme -- recommended style by @shawnborton) #### Background color, corner radius, border, and Expensify Neue font https://user-images.githubusercontent.com/1311325/223391075-17ed8fe5-b653-466d-9eec-7d3d457350b5.mp4

What alternative solutions did you explore? (Optional)

sobitneupane commented 1 year ago

@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.

PankajAS commented 1 year ago

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

sobitneupane commented 1 year ago

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

sobitneupane commented 1 year ago

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 commented 1 year ago

Components Using Picker:

PankajAS commented 1 year ago

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 commented 1 year ago

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.

flaviadefaria commented 1 year ago

What's the latest here?

bernhardoj commented 1 year ago

The picker background color is previously handled on this issue here and based on the discussions, some picker can't be customized and follow the native theme instead.

Tagging @0xmiroslav since they do the PR.

situchan commented 1 year ago

That gray background is android system modal background color in dark mode. This is not only applied to spinner but also to all dialogs including alert dialog, (In android, modal is called dialog, picker is called spinner) This seems related to https://github.com/Expensify/App/issues/13951 @shawnborton's comment to keep native theme (see also following 2 comments).

Here's another mWeb issue which was closed as not bug: #15467

So I vote to close this issue considering not a bug.

redstar504 commented 1 year ago

@shawnborton asked whether we can control the gray background of the popups on Android, and nobody claimed we could. In reality, it is indeed possible to control the background on Android, although it's not explicitly documented anywhere. I was able to find a way to do it, so I suggest waiting for @shawnborton's opinion on whether he would prefer a different background color for Android popups including the picker, since they are possible to customize beyond the default gray.

Comment for reference: https://github.com/Expensify/App/issues/13951#issuecomment-1420888395

shawnborton commented 1 year ago

Oh nice, that sounds great to me. I think I would be curious to see what our darkest app BG color looks like for the popup BG color, assuming we can also control the overlay color as well? Ideally we can make this look as close to our default modal styles as possible:

image
redstar504 commented 1 year ago

Hey @shawnborton, I think I've gotten pretty close to the look of the default app modals. I was able to implement the Expensify Neue font family and get the border radius working along with the background color, which definitely gives it a more polished feel. Unfortunately, there aren't many customization options for the overlay beyond basic transparency. For that shade of green, I reduced the transparency of the overlay to make the picker stand out more from the app background. The emulator does have a limited color space, so the dark green color may not appear as an exact #061B09 in my example. However, on a real device it should be more accurate. I'll provide a screen recording so you can view an example. Let me know if you would like me to make any further adjustments.

https://user-images.githubusercontent.com/1311325/223130116-1c6d70df-2207-47f7-90fa-7c5cb3540f70.mp4

shawnborton commented 1 year ago

Nice! Since we can't control the overlay much, can we try these two options and see how they look:

Thoughts on that?

flaviadefaria commented 1 year ago

Have we decided on a proposal? Do we have a contributor to assign to the issue?

redstar504 commented 1 year ago

I think it looks pretty good, @shawnborton! If you want me to try anything else let me know.

https://user-images.githubusercontent.com/1311325/223391075-17ed8fe5-b653-466d-9eec-7d3d457350b5.mp4

0xmiros commented 1 year ago

@redstar504 does it also apply to other native modals? And from your video, it seems ripple effect lost and doesn't look like native theme.

4

android-datepicker

And ideally, it should also change splash background color and background behind keyboard:

Screenshot_20230307_202545_New Expensify

https://user-images.githubusercontent.com/97473779/223396918-d94b830f-c158-42eb-be5c-e732f94cf25c.mov

redstar504 commented 1 year ago

@0xmiroslav It turns out that the ripple effect wasn't lost, I was just clicking too quickly. Regarding the other Android system elements, they are not affected by my change. I thought this issue was solely about the picker? It is possible to change the color scheme of those other elements. Android provides a quite a bit of style customization.

https://user-images.githubusercontent.com/1311325/223413444-67ab606b-dd57-4bda-b3bb-ab5a9c7f9d89.mp4

shawnborton commented 1 year ago

Nice, that does look great.

It is possible to change the color scheme of those other elements. Android provides a quite a bit of style customization.

Yeah, fair point that this issue was only about the picker BG but if we can also change things like datepicker, etc - might be worth adding that in as well!

flaviadefaria commented 1 year ago

Do we have an update here?

MelvinBot commented 1 year ago

@chiragsalian @sobitneupane @flaviadefaria this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

situchan commented 1 year ago

Proposal

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

android picker modal background doesn't match our dark theme color

What is the root cause of that problem?

android dialog is using gray color as background which is default color of system dark theme

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

Customize default dialog color in xml. Here, add new property - colorBackgroundFloating https://github.com/Expensify/App/blob/c5ab096c574a1e888afb67635835aeb551a6cfcf/android/app/src/main/res/values/styles.xml#L7-L12

Reference: https://developer.android.com/reference/android/R.attr#colorBackgroundFloating As doc says, this is default color of all floating components across the app in android

Addons:

DialogTheme will look like this: (color can be defined in colors.xml)

    <style name="DialogTheme" parent="Theme.AppCompat.Dialog">
        <item name="android:background">#1A3D32</item>
    </style>

These themes are fully customizable as per our design needs.

Demo: cc: @shawnborton used #1A3D32 for background color. can be changed as per requirement.

picker:

https://user-images.githubusercontent.com/108292595/224391047-56ef95ae-cb64-425b-8db6-36bec205215b.mov

datepicker:

https://user-images.githubusercontent.com/108292595/224391069-89fca766-716a-42c9-8446-ef46350615bf.mov

alert:

https://user-images.githubusercontent.com/108292595/224391089-970de9cb-6357-4289-98cd-bac8f34147fc.mov

redstar504 commented 1 year ago

@situchan Your proposal is no different than what I proposed 2 weeks ago: https://github.com/Expensify/App/issues/15460#issuecomment-1444500449

situchan commented 1 year ago

@situchan Your proposal is no different than what I proposed 2 weeks ago: #15460 (comment)

No, it can't be. The main challenge is what property to use in xml. You didn't provide it (not sure why).

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

Most developers basically know this and submit such proposal within 1 min after checking this issue.

redstar504 commented 1 year ago

@situchan I posted a screenshot of the change being applied in my proposal, so it was not a random guess. And since then @shawnborton said he wants the Picker to appear more like the modals, so I have adapted it by using popupTheme rather than colorBackgroundFloating, allowing us to apply the Expensify font family and a border as was suggested.

situchan commented 1 year ago

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.

@sobitneupane asked above here but I don't find any comment that answers to this.

redstar504 commented 1 year ago

@situchan He asked where I would make the changes, and I replied here: https://github.com/Expensify/App/issues/15460#issuecomment-144739924. If @sobitneupane would have followed up by asking what property I used, or for other additional details, I would have provided them. We have not heard from him in several days.

@chiragsalian @flaviadefaria Is it possible we can assign somebody who will attend to this issue? The price has repeatedly doubled because there has been no feedback. Perhaps @shawnborton can be involved since he has provided his input about the requested changes? We already landed on a Picker design that he is happy with.

0xmiros commented 1 year ago

@sobitneupane maybe mostly unavailable (travelling for ECX or something) these days (https://expensify.slack.com/archives/C02NK2DQWUX/p1678099798621979). As I already have full info while working on https://github.com/Expensify/App/issues/13951, I am happy to take this issue if @sobitneupane agrees. cc: @chiragsalian @flaviadefaria

redstar504 commented 1 year ago

We have not heard from him in 12 days. Did he forget the laptop? :sweat_smile:

sobitneupane commented 1 year ago

I was away for last 1 week. I will be reviewing proposals today.

sobitneupane commented 1 year ago

Thanks for proposals and discussion everyone.

Proposal from @redstar504 looks good to me. @redstar504 can you please update the proposal based on the discussions followed by the proposal so that everyone can understand the solution without going through whole discussion.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed cc: @chiragsalian @shawnborton

redstar504 commented 1 year ago

Thanks @sobitneupane. I have updated my proposal accordingly.

MelvinBot commented 1 year ago

@chiragsalian, @sobitneupane, @flaviadefaria Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

sobitneupane commented 1 year ago

@chiragsalian Can you please review this.

MelvinBot commented 1 year ago

@chiragsalian @sobitneupane @flaviadefaria this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

MelvinBot commented 1 year ago

Upwork job price has been updated to $4000

chiragsalian commented 1 year ago

Yup, sorry for the delay, ECX was extremely time-consuming. i am OOO next week but i should be able to check for status updates most days.

lol looks like sobit was also OOO for a week earlier and hence this got delayed further πŸ˜‚

Okay, so yes the proposal looks good to me. I like the popupTheme as well. Feel free to create a PR @redstar504.

MelvinBot commented 1 year ago

πŸ“£ @redstar504 You have been assigned to this job by @chiragsalian! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

chiragsalian commented 1 year ago

So @flaviadefaria, can you revert the price of this issue to $1000.

MelvinBot commented 1 year ago

Upwork job price has been updated to $1000

redstar504 commented 1 year ago

It seems like $2000 would be a more fair price here. My proposal at $1000 only included a simple background color for the picker. It was only after the discussions with Shawn when the price was $2000 that I agreed to completing a more complex theme. The scope was also expanded to include themes for the alert dialog and the date picker.

Thoughts @sobitneupane @chiragsalian?