-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Create a group chat info view with participants information. #2178
Create a group chat info view with participants information. #2178
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! This is looking great! Thanks for your contribution. I've left some comments here, some little stuff, some questions (I'm not too familiar with this part of our codebase), and some other stylistic ideas. I'm gonna go ahead and request another reviewer for redundancy.
src/pages/ParticipantsPage.js
Outdated
}, | ||
]; | ||
|
||
return final.map(item => ({...item, participantsList: [item]})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding the participantsList
key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OptionsList
component seems to use the text to display from within the participantsList
key. It was pretty weird for me too, but it threw error without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @npsedhain thanks for your patience while I was looking into this. Here were my concerns:
- We're sending two copies of the same data, nested in itself
- We're putting a single object into an array just to satisfy a proptype rule
These two concerns acted as a red flag for me, indicating that we were holding something wrong.
I started my investigation by looking at the other place in which OptionsList
is used. The items that are usually in sections.data
are not single people but whole reports (chats). While Options list looks like the correct component for the job, it was actually written with a different purpose in mind.
Two potential avenues forward:
- Don't worry about it. It works so whatever.
- Write a component that's actually meant for this.
To help us suss this out, I'd like to include @marcaaron in this decision. He wrote much of OptionsList
and is very knowledgeable in the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Luke9389, I will the changes as suggested by @marcaaron, i.e., only sending the two keys needed.
@Luke9389, I have pushed the recommended changes. Please feel free to go through it again and let me know if you have any more suggestions! |
Will do, thanks @npsedhain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. Agreed with @Luke9389 that the duplicated participantsList
is pretty weird and should be modified.
Also, just a note that the report view disappears from the background when navigating from the participants page to the details page, which isn't ideal.
Screen.Recording.2021-04-01.at.4.30.04.PM.mov
src/pages/ParticipantsPage.js
Outdated
const {icons, participants, reportName} = report; | ||
const displayNames = reportName.split(', '); | ||
|
||
const members = participants.reduce((list, login, idx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we prefer to use non-abbreviated variable names wherever possible so idx
should be index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NikkiWines, regarding the report view disappearing when navigating to the details page, I found it to be a little weird as well. But when I navigated across the application, it occurred to me that it was an existing problem. I can take a look at what exactly is causing that issue.
For example, also happens in the route /iou/request
src/libs/Navigation/linkingConfig.js
Outdated
@@ -67,6 +67,12 @@ export default { | |||
Details_Root: ROUTES.DETAILS_WITH_LOGIN, | |||
}, | |||
}, | |||
Participants: { | |||
initialRouteName: 'Participants_Root', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for initialRouteName
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's required as well, thanks for pointing it out.
👋 Couple of thoughts:
(Granted, when we add more settings/functionality to the Details side modal in the future, we may have a sub-page for the participants list, but even then my vote would be to refer to them as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can’t comment on the thread below for some reason to respond to @Luke9389 but to answer the questions...
- This is the correct component to use for this and we should not write a new one.
- The
participantsList
thing definitely looks weird / not ideal and I traced this back to this PR…
https://github.com/Expensify/Expensify.cash/pull/1632/files#r594612327
It probably doesn't matter that much that we have some weird data nesting going on here. It doesn't make much sense to me, but I don't see it hurting anything. It's used here to create tooltips which I'm assuming we will also want here for consistency.
So if we want to lean this out a bit we can pass only [{login, displayName}]
here instead.
src/pages/ParticipantsPage.js
Outdated
}).isRequired, | ||
|
||
// The chat priority mode | ||
priorityMode: PropTypes.string.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I don't think we need to know this...
src/pages/ParticipantsPage.js
Outdated
}, | ||
reports: { | ||
key: ONYXKEYS.COLLECTION.REPORT, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need to subscribe to a single report here's an example of where we do that
The function can access props
in it's argument so you can pull off the reportID
via the route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for helping, will refactor this one to listen to a single report.
src/pages/ParticipantsPage.js
Outdated
hideSectionHeaders | ||
showTitleTooltip | ||
disableFocusOptions | ||
optionMode={priorityMode === CONST.PRIORITY_MODE.GSD ? 'compact' : 'default'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really only relevant in the side bar and can maybe just be set to 'default'
here to help simplify things.
src/pages/ParticipantsPage.js
Outdated
alternateText: personalDetail.login, | ||
keyForList: `${participants.length}`, | ||
tooltipText: personalDetail.login, | ||
displayName: personalDetail.displayName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can take this out and everything about myPersonalDetails
. There's really no need to include the current user here because:
- They know already that they're a participant in the chat
- They should be accessing their profile via settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcaaron, I looked at a couple of chat applications and found them to have the individual's information listed there too. I can totally remove it if that's not something we want it in here.
src/pages/ParticipantsPage.js
Outdated
const members = participants.reduce((list, login, idx) => { | ||
list.push({ | ||
alternateText: login, | ||
displayName: displayNames[idx], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but it's not really the correct way to access these display names and will break in a future where report names are not structured as they are today.
Instead we should subscribe to the personalDetails
key. Then grab everything we need like this:
const userPersonalDetails = props.personalDetails[login];
const displayName = userPersonalDetails.displayName;
const avatar = userPersonalDetails.avatar;
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion, will make the change.
src/pages/ParticipantsPage.js
Outdated
const {icons, participants, reportName} = report; | ||
const displayNames = reportName.split(', '); | ||
|
||
const members = participants.reduce((list, login, idx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use _.reduce()
and _.map()
etc for consistency.
src/pages/ParticipantsPage.js
Outdated
<View | ||
pointerEvents="box-none" | ||
style={[ | ||
styles.detailsPageContainer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than use a page container style that belongs to another page (DetailsPage
) we should make this style more generic so it makes sense to use in both contexts.
src/pages/ParticipantsPage.js
Outdated
styles.detailsPageContainer, | ||
]} | ||
> | ||
{ participants.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an empty space here before participants
src/pages/home/HeaderView.js
Outdated
@@ -83,6 +83,9 @@ const HeaderView = (props) => { | |||
if (participants.length === 1) { | |||
Navigation.navigate(ROUTES.getDetailsRoute(participants[0])); | |||
} | |||
if (participants.length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than check if participants.length > 1
we can just return early when the length === 1
then in all other cases navigate to the new view.
@marcaaron, I have implemented the details page navigation as per your suggestion. Let me know if you have any concerns. This should solve the issue of the details page exiting the stack which used to make the report page disappear. Screen.Recording.2021-04-06.at.9.17.45.AM.mov |
@npsedhain Thanks, but it's strange that we cannot go back to view the full list again? We should add a |
Yep, I agree we should add a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more comments on this one. Looking better :)
<DetailsModalStack.Screen | ||
name="Details_Root" | ||
headerShown={false} | ||
path={ROUTES.REPORT_ID} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need this.
component={DetailsPage} | ||
options={{ | ||
...defaultSubRouteOptions, | ||
title: 'ParticipantsParticipant', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will show in a browser tab as ParticipantsParticipant
since title
sets this text as the webpage title.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a better format would be
Report > Participants > {display name of participant}
But I'm not sure we need to figure this out right now and can maybe just leave out the title entirely.
@@ -79,6 +81,31 @@ const DetailsModalStackNavigator = () => ( | |||
</DetailsModalStack.Navigator> | |||
); | |||
|
|||
const ParticipantsModalStackNavigator = () => ( | |||
<ParticipantsModalStack.Navigator | |||
path={ROUTES.PARTICIPANTS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure these path props are not doing anything and the "path" concept is left over from something we have since removed.
src/ROUTES.js
Outdated
PARTICIPANTS: 'participants', | ||
PARTICIPANTS_WITH_REPORT: 'participants/:reportID', | ||
getParticipantsRoute: reportID => `participants/${reportID}`, | ||
REPORT_ID: '/:reportID', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like any route we are using...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the path prop didn't seem to help out there. Will remove this. Thanks for pointing out.
src/ROUTES.js
Outdated
getParticipantsRoute: reportID => `participants/${reportID}`, | ||
REPORT_ID: '/:reportID', | ||
PARTICIPANTS_PARTICIPANT_DETAIL: '/participants/:reportID/:login', | ||
getParticipantsPartipantDetail: (reportID, login) => `/participants/${reportID}/${login}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"participants participant detail" could be more descriptive... e.g. maybe should be REPORT_PARTICIPANTS
and REPORT_PARTICIPANT
.
As for the routes, is there any reason we can't make these routes...
/r/:reportID/participants
/r/:reportID/participants/:login
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup we can do this, thanks for the suggestion.
@@ -34,6 +34,7 @@ import { | |||
IOUBillStackNavigator, | |||
IOURequestModalStackNavigator, | |||
DetailsModalStackNavigator, | |||
ParticipantsModalStackNavigator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd update all instances of Participants*
to ReportParticipants
so that it's clearer what we are dealing with.
name="Participants_Root" | ||
component={ParticipantsPage} | ||
options={{ | ||
...defaultSubRouteOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to pass these to the navigator instead and prevent having to add this stuff to every screen.
There are some conflicts here as well that need to be resolved before we can merge this. |
Regarding the |
@Luke9389 pretty sure this is built into the header component already https://github.com/Expensify/Expensify.cash/blob/b6d7d60e0d59c68ddf16f1c437033c77f6c8f9e7/src/components/HeaderWithCloseButton.js#L24-L25 Just want to make sure we are not reimplementing behavior we already have, thanks! |
@marcaaron Thanks for the feedbacks, will implement the changes first thing tomorrow and will let you know. |
1a8df7c
to
c0592bf
Compare
@marcaaron I have made all the recommended changes, let me know if there is anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just have a few more minor comments. Looks really nice!
@@ -82,8 +82,6 @@ module.exports = run; | |||
|
|||
const _ = __nccwpck_require__(4987); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem like these files are supposed to be here...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually didn't add this. Seem to have been added by @deetergp, a week ago in another PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely you merged in the main branch and didn't fix conflicts correctly. Regardless, this file should not be showing with changes in this PR and will need to be fixed before merging.
/> | ||
<DetailsModalStack.Screen | ||
name="Details_Root" | ||
headerShown={false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary because this was passed to navigator screenOptions
already?
src/libs/Navigation/linkingConfig.js
Outdated
Participants: { | ||
screens: { | ||
Participants_Root: ROUTES.REPORT_PARTICIPANTS, | ||
Details_Root: ROUTES.REPORT_PARTICIPANT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker but this should probably be Participants_Details
to follow a similar convention as the settings modal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now it is a blocker because I think we cannot have Details_Root
existing in two navigators? Can we change this to:
ReportParticipants_Root
ReportParticipants_Details
src/pages/DetailsPage.js
Outdated
@@ -38,11 +38,13 @@ const DetailsPage = ({personalDetails, route}) => { | |||
<HeaderWithCloseButton | |||
title="Details" | |||
onCloseButtonPress={Navigation.dismissModal} | |||
shouldShowBackButton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/pages/ReportParticipantsPage.js
Outdated
* Returns all the participants in the active report | ||
* | ||
* @param {Object} report The active report object | ||
* @param {Array} personalDetails The personal details of the users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an Object
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments but overall really coming together
src/pages/ReportParticipantsPage.js
Outdated
const propTypes = { | ||
/* Onyx Props */ | ||
// The personal details of the person who is logged in | ||
personalDetails: PropTypes.shape({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have personalDetailsPropType.js
which you can import instead of redefining the props here.
I have pushed changes including all the recent comments. Please, do let me know if there is more. |
@@ -82,8 +82,6 @@ module.exports = run; | |||
|
|||
const _ = __nccwpck_require__(4987); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely you merged in the main branch and didn't fix conflicts correctly. Regardless, this file should not be showing with changes in this PR and will need to be fixed before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the modifications to the .github/
files, this is looking good. Had one minor question about using login
vs userLogin
src/pages/ReportParticipantsPage.js
Outdated
alternateText: userLogin, | ||
displayName: userPersonalDetail.displayName, | ||
icons: [userPersonalDetail.avatar], | ||
keyForList: login, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to not use userLogin
here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, just thought using it for the key would be fine, but will do it for consistency's sake. Thank you for the suggestion.
@marcaaron, I have fixed the changes shown in the github actions folder and resolved conflicts. |
@npsedhain looks like there are still some conflicts here. |
@NikkiWines Will fix them ASAP. |
@NikkiWines Done! |
src/libs/Navigation/linkingConfig.js
Outdated
Participants: { | ||
screens: { | ||
Participants_Root: ROUTES.REPORT_PARTICIPANTS, | ||
Details_Root: ROUTES.REPORT_PARTICIPANT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now it is a blocker because I think we cannot have Details_Root
existing in two navigators? Can we change this to:
ReportParticipants_Root
ReportParticipants_Details
src/pages/DetailsPage.js
Outdated
@@ -37,12 +40,14 @@ const DetailsPage = ({personalDetails, route}) => { | |||
<ScreenWrapper> | |||
<HeaderWithCloseButton | |||
title="Details" | |||
shouldShowBackButton={!!route.params.reportID} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like an implementation detail. Can we make this more explicit by adding something above like:
// If we have a reportID param this means that we arrived here via the ParticipantsPage and should be allowed to navigate back to it
const shouldShowBackButton = Boolean(route.params.reportID);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, but I'm seeing an issue where I can't see the details page for an account with an SMS login though their details do exist. Looks like it's because of login: userLogin
, which should be using login
in that case. My bad for not catching that when I recommended that change earlier.
Screen.Recording.2021-04-13.at.11.05.29.AM.mov
Screen.Recording.2021-04-13.at.11.06.38.AM.mov
@marcaaron @NikkiWines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience and for all the changes. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍
All yours @Luke9389 |
Thanks for helping with the reviews @marcaaron and @NikkiWines |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Thanks @npsedhain - we will issue payment on 21 April assuming no regressions. |
Thanks @laurenreidexpensify, but I think you tagged the wrong person. 😅 |
Sorry @npsedhain!!! My bad 🤦🏽♀️ |
🚀 [Deployed](https://github.com/Expensify/Expensify.cash
|
Details
Added a modal view to show the list of all the participants in the group chat.
Fixed Issues
Fixes GH_LINK_ISSUE_2105
Tests
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android