Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Conversation

@Sinharitik589
Copy link
Contributor

@Sinharitik589 Sinharitik589 commented Apr 16, 2022

Fixes element-hq/element-web#21794

Signed-off-by: Ritik Kumar [email protected]


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

Preview: https://pr8344--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@Sinharitik589 Sinharitik589 requested a review from a team as a code owner April 16, 2022 07:05
@Sinharitik589 Sinharitik589 marked this pull request as draft April 16, 2022 08:58
@Sinharitik589 Sinharitik589 marked this pull request as ready for review April 16, 2022 15:16
@t3chguy t3chguy changed the title fixes #21794 , preventing invites from showing up on breadcrumbs Prevent invites from showing up in breadcrumbs Apr 19, 2022
@codecov
Copy link

codecov bot commented Apr 22, 2022

Codecov Report

Merging #8344 (b53bff1) into develop (763edb7) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #8344      +/-   ##
===========================================
- Coverage    30.47%   30.47%   -0.01%     
===========================================
  Files          889      889              
  Lines        50547    50551       +4     
  Branches     12861    12864       +3     
===========================================
  Hits         15405    15405              
- Misses       35142    35146       +4     
Impacted Files Coverage Δ
src/stores/BreadcrumbsStore.ts 12.79% <0.00%> (-0.63%) ⬇️

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

this requires product review as it is currently a feature, not a bug

@turt2live turt2live requested a review from a team April 26, 2022 04:45
Copy link
Contributor

@kittykat kittykat left a comment

Choose a reason for hiding this comment

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

Looks good from the product side 👍

@Sinharitik589 Sinharitik589 requested a review from turt2live May 1, 2022 09:19
@turt2live turt2live removed their request for review May 2, 2022 01:07
Comment on lines 87 to 88
const membership = room?room.getMyMembership():"";
if (room && membership==='join') await this.appendRoom(room);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const membership = room?room.getMyMembership():"";
if (room && membership==='join') await this.appendRoom(room);
const membership = room?.getMyMembership();
if (membership === "join") await this.appendRoom(room);

Comment on lines +90 to +92
} else if (payload.action === Action.JoinRoom) {
const room = this.matrixClient.getRoom(payload.roomId);
if (room) await this.appendRoom(room);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm why do we need to add this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line checks whether the user has successfully joined the room or not , then only we are adding that particular room to the breadcrumbs.

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@germain-gg germain-gg merged commit d717d06 into matrix-org:develop May 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Invites" show up in breadcrumb

5 participants