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

Add a Settings page to configure people in workspace #3681

Merged
merged 62 commits into from
Jun 30, 2021

Conversation

mountiny
Copy link
Contributor

@mountiny mountiny commented Jun 18, 2021

Details

This PR adds a workspace configuration page where admins can remove people from their workspaces or use modal to invite new members.

cc @marcaaron @MitchExpensify @Jag96 @madmax330

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/166468

Tests

  1. Create a free workspace policy via the global create menu
  2. Navigate to the settings screen and verify this workspace is visible
  3. Tap the workspace
  4. Tap the "People" option on the next screen
  5. If there is only you, the admin, in the workspace, use the green Invite button which will bring in a modal from a side. Enter an email of an account you want to add to the workspace.
  6. Once you added the number of accounts to the workspace you would wish, select some using the checkboxes on the left side. As soon as you select one checkbox, the Remove button should turn bright red. Click on the Remove button.
  7. A confirmation modal looking like this should show up. Click Remove again to confirm the removal.
    image
  8. Make sure the person selected has been removed from the list and it does not show up even after page refresh.
  9. Now, try selecting the first uppermost checkbox and make sure that all the checkboxes has been checked.
  10. Go on and try to remove all of the members of the workspace.
  11. Your account, the admin, should stay unremoved after this action. That is the correct behaviour, you cannot remove admin of workspace through this page.

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

2021-06-30_10-43-36

Mobile Web

2021-06-30_10-48-56

Desktop

2021-06-30_10-53-24

iOS

2021-06-30_10-41-48

Android

2021-06-30_10-55-46

@mountiny mountiny self-assigned this Jun 18, 2021
@mountiny mountiny requested a review from a team as a code owner June 18, 2021 18:53
@MelvinBot MelvinBot requested review from TomatoToaster and removed request for a team June 18, 2021 18:53
@mountiny
Copy link
Contributor Author

I will be making progress on this one over the weekend so I can ask clarifying questions on Monday.

@mountiny
Copy link
Contributor Author

@marcaaron Sorry for polluting the PR with merged commits from Rajat's branch. Should I squash the commits to one or can I leave it like that?

@MitchExpensify Progress update:

I have shamelessly used great work from Rajat and Joe. I have added the People page, so far with title, tagline and the two buttons where only Invite button does what it should so far.

I will need to update the src/styles/themes/default.js with appropriate buttonDangerBG and buttonDangerHoveredBG and anything needed there as E.cash does not have red themed buttons yet.

I have set up state property selectedEmployees, which will be used to keep track of the people checked to be potentially removed.

Tomorrow I will work on the table and remove functionality.

@mountiny mountiny changed the title [WIP] Add a new route [HOLD parasharrajat:workspace] Add a Settings page to configure people in workspace Jun 21, 2021
@mountiny
Copy link
Contributor Author

@marcaaron I guess with the commit history, we can just leave it like this and once Rajat's PR will be approved and merged into main, this diff will disappear.

@mountiny mountiny force-pushed the vit-settingsPageToConfigureWorkspace branch 2 times, most recently from 2a05467 to 00a1071 Compare June 21, 2021 14:04
@marcaaron
Copy link
Contributor

marcaaron commented Jun 21, 2021

@marcaaron Sorry for polluting the PR with merged commits from Rajat's branch. Should I squash the commits to one or can I leave it like that?

Squashing commits is fine as long as reviews have not yet started.

@marcaaron I guess with the commit history, we can just leave it like this and once Rajat's PR will be approved and merged into main, this diff will disappear.

I would maybe suggest we take out the unmerged changes from Rajat's PR and set up this view as as standalone page for now. We can always tie everything together later and it would make the changes here easier to review (or we can wait until Rajat's changes are merged, but that seems less ideal).

@marcaaron marcaaron self-requested a review June 21, 2021 17:45
@mountiny
Copy link
Contributor Author

@marcaaron
Update: I have used the InvertedFlatList to create the list of employees. The styling will definitely need some polishing to match the proposed design.

Can you please give me a hint what is the best way for writing something like @media queries as we need to change the cell design slightly for mobile?

For now, I have included mockData for easier troubleshooting of the design and in case you do not have the correct policyId yet. But it works with employeeList too.

Next, I will work on adding the the Remove onyx functionality. The checkboxes on page should already work. i will also look into taking the Rajat changes out.

@shawnborton I thought you might be the best one to ask regarding design, for the Admin badge, there is the little tick. Is that already somewhere in code or is it something new I should add. In that case, would you have an example of something similar? Thank you!

@marcaaron I wanted to ask for a clarification to this last bullet point of the doc:

A badge that indicates the admin (hint: it’s only the current user for now and will be used from ONYXKEYS.SESSION)

I cannot see anything helpful to find out if the user is admin in SESSION. Is that something which is being implemented somewhere else? Or who should I show the admin badge at now? Thank you!

Thank you guys for any feedback on the code so far!

image

image

@marcaaron
Copy link
Contributor

Update: I have used the InvertedFlatList to create the list of employees. The styling will definitely need some polishing to match the proposed design.

We probably can just use the regular FlatList that comes with React Native as we don't need to invert the content for this case (only for a chat and anything that has content from the bottom to top).

Can you please give me a hint what is the best way for writing something like @media queries as we need to change the cell design slightly for mobile?

https://github.com/Expensify/Expensify.cash/blob/main/src/components/withWindowDimensions.js

I cannot see anything helpful to find out if the user is admin in SESSION. Is that something which is being implemented somewhere else? Or who should I show the admin badge at now? Thank you!

We can access the current user's login via session key and then compare to the admin email. For example, here we check to see if the currently logged in user has a "phone login" so we can do something similar to that...

https://github.com/Expensify/Expensify.cash/blob/c1536a83ef628f27dcae5d0320bfba16d7bff58e/src/pages/ReimbursementAccount/ReimbursementAccountPage.js#L112

@shawnborton
Copy link
Contributor

@mountiny the asset already exists as an .svg, it's probably named checkmark.svg

There is some other design feedback I have too about the overall style of the modal, but I know we have another contributor working on some of these styles for the Expensify Card stuff. @marcaaron what do you recommend we do here? Wait and do a follow up?

@marcaaron
Copy link
Contributor

Sounds fine as long as the focus is on what's inside the modal @shawnborton? The rest we can cover with Rajat since we should be removing the code from here so we can tackle the relevant changes in each. So just this area and not the overall style of the modal.

2021-06-21_15-26-07

@shawnborton
Copy link
Contributor

Cool, that works for me!

@mountiny
Copy link
Contributor Author

@shawnborton Feel free to leave the design feedback here, I will tackle this first thing in a morning tomorrow and hopefully get close to finish!

@shawnborton
Copy link
Contributor

Things look good to me. But actually @michelle-thompson can you remind me, what do we need the checkmark for in the little Admin pill?

@michelle-thompson
Copy link
Contributor

I think we actually decided against that since this table will only show who's already been added to the workspace.

@shawnborton
Copy link
Contributor

Oh nice, that was easy. Mind adding an up-to-date mock here when you get a chance?

@michelle-thompson
Copy link
Contributor

Hmm, none of the above mockups have checkmarks in the pills though?

Here it is in case I'm missing something:
image

@shawnborton
Copy link
Contributor

Check out the linked issue though: https://github.com/Expensify/Expensify/issues/166468 You will see the checkmark there

@marcaaron marcaaron self-assigned this Jun 30, 2021
@TomatoToaster
Copy link
Contributor

Boom! It's working very nicely now. I am testing it with the banner here (right now it won't work on localhost on the master branch, but I'm updating it in this PR:https://github.com/Expensify/Web-Expensify/pull/31302 and will test it when that one goes live)
image

image

@@ -22,7 +22,7 @@ const optionPropTypes = PropTypes.shape({
participantsList: PropTypes.arrayOf(participantPropTypes),

// The array URLs of the person's avatar
icon: PropTypes.arrayOf(PropTypes.string),
icons: PropTypes.arrayOf(PropTypes.string),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍🏾

@shawnborton
Copy link
Contributor

Another small design comment: maybe my eyes are playing tricks on me, but can someone confirm that the border width of the lines above & below the user rows are 1px? Same with the border divider between the left pane and right pane - is that 1px wide? Optically it feels like it might be 2 so just want to double check.

TomatoToaster
TomatoToaster previously approved these changes Jun 30, 2021
@mountiny
Copy link
Contributor Author

mountiny commented Jun 30, 2021

@shawnborton The divider between left and main part is actually 2px wide and the row dividers are also 2px wide.

Should it all be only 1px?

@shawnborton
Copy link
Contributor

Yup - they should all be 1px, thanks!

@mountiny
Copy link
Contributor Author

Gonna update it!

@marcaaron marcaaron self-requested a review June 30, 2021 22:23
@marcaaron
Copy link
Contributor

Gonna merge this since it's blocking some other things + the only remaining blocker was the border which is fixed. Thanks for the reviews @TomatoToaster !!

@marcaaron marcaaron merged commit 72b8e1d into main Jun 30, 2021
@marcaaron marcaaron deleted the vit-settingsPageToConfigureWorkspace branch June 30, 2021 22:25
@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.

@roryabraham
Copy link
Contributor

Unfortunately the deploy comments are not working right now. This was deployed to staging yesterday.

@kavimuru
Copy link

kavimuru commented Jul 9, 2021

Android - App is crashing after entered an email of an account to add to the workspace

Expected Result:

Able to add the number of accounts to the workspace

Actual Result:

App is crashing after entered an email of account

Actions Performed:

  1. Lunch the app
  2. Log in with expensifail account
  3. Click on Plus button and Select Workspace
  4. Tap the workspace
  5. Tap the "People" option on the next screen
  6. use the green Invite button which will bring in a modal from a side.
  7. Enter an email of an account you want to add to the workspace

Platform:

iOS
mWeb
Android ✔️

Build:

1.0.76-0

Notes/Images/Video:

Bug5144843_Screenshot_20210708-220331_One_UI_Home

Bug5144843_Screen_Recording_20210708-220332_Expensifycash.mp4

log file.txt

@Jag96
Copy link
Contributor

Jag96 commented Jul 9, 2021

Looks like this is caused by this line because policy.employeeList is undefined for this flow. I'll move this to an issue and make a PR for this.

@s77rt
Copy link
Contributor

s77rt commented May 18, 2023

This PR caused a bug #17166. The WorkspacePeoplePage suffers from stale data issue.
Selected employees are stored in a state that is filled with values from policy.employeeList prop but it does not react to changes to that prop e.g. if we clear policy.employeeList the selectedEmployees array would still hold values.

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.

10 participants