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

Report Details and Settings for User Created Policy Rooms #7028

Merged
merged 68 commits into from
Jan 12, 2022

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Jan 4, 2022

CC: @jasperhuangg, @yuwenmemon

Details

Configures the report details page as well adding the new report settings page for User Created Policy Rooms.

Also adds a buttonStyle feature to the Button component since previously it would not let you set custom sizes/padding for the actual button portion of the component.

Also componentizes the TextInput used for RoomName, this way it'll work with both WorkspaceNewRoomPage.js and ReportSettingsPage.js. Since they both care about the changing text (i.e. roomName) as well as the error, I had the RoomNameInput component bubble up whenever the text OR error changes (see componentDidUpdate). This was the best way I could find to share functionality in RoomNameInput, while also letting each component handle the roomName in their own way. RoomNameInput also correctly checks against other reports within a given policyID for name changes. I've tested that this works in WorkspaceNewRoomPage as well where it will prevent you from making Policy Rooms with duplicate names on the same Workspace.

The the Invite, Leave Room and the Save button in the Settings, Room Name sections are non-functional at the moment, I've replaced them with /* Placeholder Comments */ for now since they need to be built and added to Web-E/Auth in other PRs. This PR being merged would unblock those other features.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/176851

Tests/ QA Steps

On an account that already has a workspace

  1. Create a New Room using the plus in the bottom left of the screen. If you already have Policy Rooms, verify that it won't let you create one with the same name on the same workspace.
  2. After it is created, click on the Report Details at the top (if it crashes immediately after creating the room, that's caught and being solved in this issue, continue after reloading/reopening the app).
  3. Verify you see something that matches the screenshots for the Policy Room.
  4. Find a defaultRoom (#admins or #announce will work) and verify it also matches the screenshots for Default Room.

Note that the Invite, Leave Room and the Save button in the Settings, Room Name sections are non-functional at the moment. They will be added in later PRs, this PR just sets those features up visually.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Policy Room Details:
image

Policy Room Settings (Restricted):
image

Policy Room Settings (Private):
image

Default Room Details:
image

Default Room Settings:
image

@TomatoToaster TomatoToaster self-assigned this Jan 4, 2022
@TomatoToaster TomatoToaster force-pushed the amal-policy-rooms-details-page branch 2 times, most recently from f9b80df to 0d2e0e7 Compare January 6, 2022 22:19
yuwenmemon
yuwenmemon previously approved these changes Jan 11, 2022
yuwenmemon
yuwenmemon previously approved these changes Jan 11, 2022
@TomatoToaster
Copy link
Contributor Author

TomatoToaster commented Jan 11, 2022

Ok now it's ready for another review, I addressed all of @jasperhuangg's comments too now.

For the Button styles part, I decided to rename buttonStyles to innerStyles, but held off on updating the style prop to be called containerStyles since that would need to be changed in so many places. Maybe that makes sense in its own PR, or might not even be necessary as change. If you think I should add it to this PR, let me know.

@jasperhuangg
Copy link
Contributor

I think styles and inner styles are clear enough. You're saying you'd need to change every use of Button to use containerStyles, which I don't think is necessary (also I'd wanna get a second opinion too).

Looks good!

@yuwenmemon yuwenmemon merged commit afcbb13 into main Jan 12, 2022
@yuwenmemon yuwenmemon deleted the amal-policy-rooms-details-page branch January 12, 2022 00:35
@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
Copy link
Contributor

🚀 Deployed to staging by @yuwenmemon in version: 1.1.27-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @yuwenmemon in version: 1.1.27-3 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @yuwenmemon in version: 1.1.27-3 🚀

platform result
🤖 android 🤖 skipped 🚫
🖥 desktop 🖥 skipped 🚫
🍎 iOS 🍎 skipped 🚫
🕸 web 🕸 skipped 🚫

@kavimuru
Copy link

@TomatoToaster #7055 is not fixed for Web and mWeb. Blank screen appears clicking room name to view the details.
Web - Blank screen
Mobile Web - Blank screen
Do I have to open a new issue?

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.29-5 🚀

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

translationKey: 'common.members',
icon: Expensicons.Users,
subtitle: props.report.participants.length,
action: () => { Navigation.navigate(ROUTES.getReportParticipantsRoute(props.report.reportID)); },
Copy link
Contributor

Choose a reason for hiding this comment

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

You mentioned some bugs on our call recently, and I was just writing something else in this file. I could be wrong but I think you need this.props.... here. (same on the subtitle key above and the action for settings below.

@eVoloshchak
Copy link
Contributor


Leaving a note that this PR caused a small issue in #14683, ReportSetting/Participants pages needed to be wrapped with FullPageNotFoundView in case there is no report data

@mananjadhav
Copy link
Collaborator

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

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.

None yet

8 participants