Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Group Chats] Add remaining features #39757
[Group Chats] Add remaining features #39757
Changes from all commits
9dba74f
b35fccb
bf96f01
c14f10b
66a220a
2e3e6bb
b041c73
9ed73cd
35bbf1b
9aa8f74
b7caed0
d8c6956
459bef4
5a0e7eb
e6a047d
699cfc7
8791b81
fc84939
639693b
790d3ca
b734506
8118ed1
7ac3859
61afba6
cd7ccc0
c7d89f2
98058a5
08fedfc
46c75fd
d37e029
259355f
0b56abe
a6d8ca4
09c2fd9
896f0c7
38c5934
640f90b
6c59b94
0289227
d7837d8
14aaaf1
dea4cbf
46ea41a
5dae3d1
2bdf2c8
17ad9c5
99cfe87
aae648b
fab964f
6664cfb
51ab0c2
e2663cb
c7d2d36
e202c7b
67f2986
bd0f05e
2dda3f1
9488c3b
0dd6b90
3241bbd
21fd1d6
ef4286d
51052e0
9b10486
f049283
dec226e
1dfdc64
6a74c9d
faed7df
72b4884
9568bc3
9c2ab05
def5c54
feaf901
d27b691
ba3e768
1efda68
4ee60b5
76e0d92
bc0c380
b4248e4
2acb986
69e0de5
e57ad31
01b032a
6f3c030
84e631c
351f318
d054329
22f6d15
41b3924
85454e6
06eb201
c479153
e391d7d
ae4ed17
2440183
f5970a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
question: why does this route not use
getUrlWithBackToParam
, whileDETAILS
andROLE_SELECTION
do?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.
Great question. I don't have a great understanding of
backTo
or how that works. I removed it from the routes you mentioned. If we find out we need them we can add them later.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 it's mostly for the sake of the browser back button working as expected, but last I checked it wasn't really working after the "ideal nav" changes a couple months ago.
Whatever the case, more polish than blocker
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 I'll move this into the "to check later" pile. I am eager to get this merged soon-ish.
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.
Unsure if there is a better url for this. Maybe
r/:reportID/settings/name
is better thangroup-name
🤔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, I'd think it might be simpler to combine
REPORT_SETTINGS_ROOM_NAME
andREPORT_SETTINGS_GROUP_NAME
into a singleREPORT_SETTINGS_NAME
route atr/:reportID/settings/name
that works with any report type.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.
Nice. That feels worth exploring as a follow up. Updating the Group name is a lot looser than updating the Room name. Gonna leave this for now I think.
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.
When we leave a group chat we replace the current screen with the most recent report so the group chat is no longer in stack. However in this case the function is called from a RHP screen.
If the stack is as follow:
report 1 -> report 1 details page
and we leave the group, the stack will be:
report 1 -> report 2
report 1 can still be accessed by going back (bug).
To fix this we opted to dismiss any RHP modal before proceeding
(From #44152)
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.
suggestion: we should create a follow-up issue to add UI tests for this very common component to ensure that it shows what we expect in different scenarios. i.e:
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.
Oh, we already have some that do a basic version of what you are asking for the unread indicators test, but nothing for Group Chats yet (since it's brand new). I have a lot on my plate at the moment so can't prioritize it right now. though @s77rt or another contributor might be interested in looking into this.