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

fixed Sidebar theme #1979

Merged

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Mar 22, 2021

Please review @marcaaron. As these changes are directly linked to your code.

Details

<SafeAreaInsetsContext.Consumer> does not have styling support but in ScreenWrapper component was passing parent style prop to it. I have moved it to the child View which is working without regression.

Fixed Issues

Fixes #1967

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android
Before After
image image

CC @shawnborton

@parasharrajat parasharrajat requested a review from a team as a code owner March 22, 2021 16:22
@botify botify requested review from Luke9389 and removed request for a team March 22, 2021 16:22
@parasharrajat parasharrajat changed the title Sidebar Theme is not correct. Sidebar theme fixed Mar 22, 2021
@parasharrajat parasharrajat changed the title Sidebar theme fixed fixed Sidebar theme Mar 22, 2021
@shawnborton
Copy link
Contributor

Looks good but would love to see screenshots of all of the platforms, especially iOS.

src/components/ScreenWrapper.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

@shawnborton Unfortunately, I don't have IOS. Here are screenshots for other platforms. As per your query, the Statusbar is transparent and its color depends on the screen below.

Desktop

Screenshot_20210323_005459

Mobile Web

Screenshot_2021-03-23 Expensify cash - Chat with friends to send and receive money

Android

Screenshot_2021-03-23-01-16-29-250_com expensify chat

@parasharrajat parasharrajat force-pushed the parasharrajat/Screenwrapper branch 3 times, most recently from ff32c17 to 83b16d7 Compare March 22, 2021 20:12
@parasharrajat
Copy link
Member Author

@Luke9389 Updated.
@shawnborton Any comments on the screenshots above.
Thanks.

@shawnborton
Copy link
Contributor

Let me pull this down and test on iOS for you.

@shawnborton
Copy link
Contributor

Looks good on iOS!

@Luke9389
Copy link
Contributor

@parasharrajat nice work on this PR! Sorry that my reviews may have felt a bit... nit-picky. In the future, please know that if I ask a question on a review, I really don't know the answer. So don't feel like I'm asking you rhetorical questions, or that I'm hinting at something. I'll always be direct 😄. Anyway, good job, and thank you! 🙇‍♂️

@Luke9389 Luke9389 merged commit f93683f into Expensify:master Mar 23, 2021
@parasharrajat
Copy link
Member Author

@Luke9389. I actually like the nit-picky reviews. it helps in decision-making. Eventually, a review is a kind of second personality's point of view. So tackling all these reviews is making me good at making decisions related to code. I am sure it will help in the long run when we have to work over a lib for developers and not for consumers. 👍

@laurenreidexpensify
Copy link
Contributor

@parasharrajat I will make a plan to get you paid for this PR as a bonus on your next Upwork payment 👍🏽

@parasharrajat
Copy link
Member Author

@laurenreidexpensify Amazing. Thanks

@laurenreidexpensify
Copy link
Contributor

Okay as that was merged on Tues 23 March, will pay you on 30 March as per the 7 day regression test 👍🏽

@parasharrajat parasharrajat deleted the parasharrajat/Screenwrapper branch November 4, 2022 19:59
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.

LHN background color is not correct.
4 participants