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.

Sign up for GitHub

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

Jump to bottom

Fix: Workspace settings page design #5804

Merged
merged 2 commits into from
Oct 18, 2021
Merged

Fix: Workspace settings page design #5804

merged 2 commits into from
Oct 18, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Oct 13, 2021 •
edited
Loading

Details

Proposal => #5767 (comment)

  1. Passed the footer as Prop.
  2. Refactor the UI to use a single Container View.

Fixed Issues

$ #5767

Tests | QA Steps

  1. Go to any workspace Settings from the main Settings page.
  2. Click General settings.
  3. Page layout should be correct and Button should have proper placement.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

image

Mobile Web

Screenshot 2021-10-13 16:21:57

iOS

Android

Screenshot 2021-10-13 16:20:42

@parasharrajat parasharrajat requested a review from a team as a code owner October 13, 2021 10:52
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team October 13, 2021 10:52
@parasharrajat
Copy link
Member Author

@Julesssss Ready to merge.

@Julesssss
Copy link
Contributor

Will get to this tomorrow, ran out of time today.

@parasharrajat
Copy link
Member Author

I think @Beamanator will take care of this now.

Julesssss
Julesssss previously approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

Code LGTM. Didn't get a chance to run, but this is on my list for tomorrow if it hasn't yet been merged.

Beamanator
Beamanator reviewed Oct 13, 2021
View reviewed changes
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Tests well! Only 2 tiny comments

src/pages/workspace/WorkspacePageWithSections.js Show resolved Hide resolved
src/pages/workspace/WorkspacePageWithSections.js
Comment on lines 48 to +51
children: () => {},
user: {},
reimbursementAccount: {},
footer: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB; I think it could be nice to alphabetize these props or put them in the order they appear in propTypes. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually, we keep the capital case for props that are a Component but here I am passing an element.

so if we want to use the footer as <Footer> then it will make more sense.

So, I think this is fine.

@parasharrajat @Beamanator
Update comment
659fe5a 
Co-authored-by: Alex Beaman <dabeamanator@gmail.com>
@parasharrajat
Copy link
Member Author

@Julesssss This is ready.

@Julesssss
Copy link
Contributor

tested today, looking good. Will leave the merge to @Beamanator as he's yet to approve.

@Julesssss
Copy link
Contributor

I'm going to merge, so that we can get this N6 improvement in as soon as possible.

@Julesssss Julesssss merged commit 7cd3bdb into Expensify:main Oct 18, 2021
@OSBotify
Copy link
Contributor

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

@OSBotify OSBotify mentioned this pull request Oct 23, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Julesssss in version: 1.1.8-10 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@parasharrajat parasharrajat deleted the workspace-design branch November 20, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Beamanator Beamanator Beamanator left review comments

@Julesssss Julesssss Julesssss approved these changes

Assignees

@parasharrajat parasharrajat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@parasharrajat @Julesssss @OSBotify @Beamanator