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

Extract view/join room logic to room helper #8329

Merged
merged 9 commits into from
Apr 21, 2022

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Apr 14, 2022

In preparation for element-hq/element-web#21354, we need the logic extracted to helper function so that it can be re-used across the product


This change is marked as an internal change (Task), so will not be included in the changelog.

Preview: https://pr8329--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.

@germain-gg germain-gg requested a review from a team as a code owner April 14, 2022 16:13
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.

generally looks good, just some code quality things that haven't quite made it to formal documentation yet (they exist as internal proposals, but not as public docs).

src/components/structures/RoomDirectory.tsx Outdated Show resolved Hide resolved
src/utils/rooms.ts Outdated Show resolved Hide resolved
src/utils/rooms.ts Outdated Show resolved Hide resolved
src/utils/rooms.ts Show resolved Hide resolved
src/utils/rooms.ts Outdated Show resolved Hide resolved
src/utils/rooms.ts Outdated Show resolved Hide resolved
src/utils/rooms.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #8329 (4dca8fc) into develop (6b13988) will decrease coverage by 0.00%.
The diff coverage is 2.98%.

@@             Coverage Diff             @@
##           develop    #8329      +/-   ##
===========================================
- Coverage    30.03%   30.02%   -0.01%     
===========================================
  Files          881      882       +1     
  Lines        50213    50226      +13     
  Branches     12791    12794       +3     
===========================================
+ Hits         15081    15082       +1     
- Misses       35132    35144      +12     
Impacted Files Coverage Δ
src/components/structures/RoomDirectory.tsx 0.94% <0.00%> (+0.15%) ⬆️
src/components/views/directory/NetworkDropdown.tsx 4.81% <ø> (-1.14%) ⬇️
src/utils/error.ts 0.00% <0.00%> (ø)
src/utils/rooms.ts 8.62% <1.92%> (-58.05%) ⬇️
src/utils/DirectoryUtils.ts 7.69% <100.00%> (+7.69%) ⬆️

@turt2live turt2live requested review from a team and removed request for turt2live April 20, 2022 05:14
@turt2live turt2live dismissed their stale review April 20, 2022 05:15

review was on an old diff, and may have been addressed

Copy link
Collaborator

@jryans jryans 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 overall, thanks! 😄

Comment on lines +364 to 367
Modal.createTrackedDialog(e.message, '', ErrorDialog, {
title: e.message,
description: e.description,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe utils/error.ts should offer this as showErrorModal()? I assume this could become a common pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure we can do this as the .ts files should not have anything to do with UI according to our new style guide.
So i guess we should probably have some UI helpers?

@germain-gg germain-gg added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Apr 21, 2022
@germain-gg germain-gg merged commit 5d6143a into develop Apr 21, 2022
@germain-gg germain-gg deleted the gsouquet/extract-view-room branch April 21, 2022 09:56
MadLittleMods added a commit that referenced this pull request Apr 3, 2023
Originally introduced in
#8329

All usages were removed in
#9605

This error also has problems with showing tranlsated versions in the logs.
Part of element-hq/element-web#9597

We should be using the new `UserFriendlyError` for this kind of thing anyway.
Introduced in #10440
MadLittleMods added a commit that referenced this pull request Apr 4, 2023
Originally introduced in
#8329

All usages were removed in
#9605

This error also has problems with showing tranlsated versions in the logs.
Part of element-hq/element-web#9597

We should be using the new `UserFriendlyError` for this kind of thing anyway.
Introduced in #10440
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants