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

cleanup for GSD layout #2104

Merged
merged 9 commits into from
Apr 1, 2021
Merged

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Mar 26, 2021

@marcaaron Please review.

Fixes

fixes #2123

Details

Following up on the changes mentioned #2015 (review)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Internal Code changes thus no screenshots.

@parasharrajat parasharrajat requested a review from a team as a code owner March 26, 2021 12:10
@botify botify requested review from joelbettner and removed request for a team March 26, 2021 12:10
src/pages/home/sidebar/OptionRow.js Outdated Show resolved Hide resolved
@parasharrajat parasharrajat requested a review from marcaaron March 26, 2021 21:01
@parasharrajat
Copy link
Member Author

Updated. Thanks.

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes looks much better. Have one more thought and then I'll stop bugging you 😅

src/pages/home/sidebar/OptionRow.js Outdated Show resolved Hide resolved
@shawnborton
Copy link
Contributor

@parasharrajat see my comment over here: #2123 (comment)

In case we want to fix that while you are addressing clean up items here.

@parasharrajat
Copy link
Member Author

@shawnborton Thanks for mapping that issue to the PR. It's been taken care of.

@parasharrajat parasharrajat requested a review from marcaaron March 30, 2021 18:04
@@ -920,8 +920,8 @@ const styles = {
},

focusedAvatar: {
backgroundColor: themeColors.pillBG,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These variables were removed recently so I have to migrate.

@marcaaron
Copy link
Contributor

@parasharrajat you've got some conflicts here now and please try to remove all inline styles. We should not be passing any objects directly to a style prop. If you need to return a style object based on some props create a helper method and add it to styles.js like we have done here.

https://github.com/Expensify/Expensify.cash/blob/0867cbf5882e416766e120144a9fcee20b219682/src/styles/styles.js#L1370-L1405

@parasharrajat
Copy link
Member Author

@marcaaron Updated. Thanks.

@marcaaron marcaaron merged commit 2762798 into Expensify:master Apr 1, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Apr 1, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@roryabraham
Copy link
Contributor

roryabraham commented Apr 2, 2021

I believe this PR may have caused a regression where the background between the avatars becomes black when the option is selected:

Not Selected Selected
image image

Note: reproducible on staging but not production so this should be a deploy blocker.

@parasharrajat
Copy link
Member Author

Oh. I think this PR should fix this. Let me check.

@roryabraham
Copy link
Contributor

Oops, ignore me, I was not on the latest staging version.

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.

Group creation - Not enough space between circle and edge
5 participants