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

[User Settings] Implement Settings Menu Selectors #1672

Merged
merged 8 commits into from
Mar 10, 2021

Conversation

Maftalion
Copy link
Contributor

@Maftalion Maftalion commented Mar 9, 2021

Details

Original PR #1630

Fixed Issues

Fixes https://github.com//issues/1493

Tests

  • Click into Settings page from HomePage
  • Click each menu item and confirm subroute renders

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-03-09 at 9 04 51 AM

Mobile Web

Screen Shot 2021-03-09 at 9 07 04 AM

Desktop

Screen Shot 2021-03-09 at 9 06 15 AM

iOS

Screen Shot 2021-03-09 at 9 12 17 AM

Android

Screen Shot 2021-03-09 at 9 07 58 AM

# Conflicts:
#	src/pages/SettingsPage/InitialPage.js
# Conflicts:
#	src/pages/settings/InitialPage.js
# Conflicts:
#	src/pages/settings/InitialPage.js
@Maftalion Maftalion requested a review from a team as a code owner March 9, 2021 03:16
@botify botify requested review from jasperhuangg and removed request for a team March 9, 2021 03:16
@Maftalion
Copy link
Contributor Author

Maftalion commented Mar 9, 2021

um not sure how i broke things while merging master with the original pr #1630 😅 , but i just cherry picked my commits over and opened a fresh pr
@NikkiWines @cead22 @marcaaron

Screen.Recording.2021-03-08.at.6.49.52.PM.mov

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Mar 9, 2021

Hey @Maftalion! I was randomly assigned by PullerBear and I don't think I have enough context to be reviewing this PR, especially considering how things were broken before.

It looks like @cead22 @shawnborton @NikkiWines and @marcaaron were reviewing the PR that this one was made from. I think they should probably be the ones reviewing this. I'll remove myself from the assignment.

As a note though it looks like you're failing the JSX style tests, but these seem to have been fixed in this PR: #1674.

Thanks!

@jasperhuangg jasperhuangg removed their request for review March 9, 2021 08:17
@shawnborton
Copy link
Contributor

The video you posted looks great to me, but I'd also love more screenshots in the original comment as well.

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.

LGTM

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

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

Tested and works well :shipit:

@shawnborton
Copy link
Contributor

Tested and this looks great. The only slight weirdness I see is that if you navigate to a subpage of Settings (like Preferences) and then tap on the "X" in the top right corner, you see the parent Settings page flash for a split second as the rightDocked sidepane exits.

@cead22 cead22 merged commit beda5cb into Expensify:master Mar 10, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2021
@NikkiWines
Copy link
Contributor

@shawnborton I think the view weirdness will probably be handled when we change the whole navigation system in marc's react-navigation PR #1559

@marcaaron
Copy link
Contributor

Merged master into my branch there and the animation does the flashy thing. However, I think we should next merge the react-nav stuff and iron out these details later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[User Settings] Implement Settings Menu Selectors
6 participants