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

Remove old Room Directory #9056

Closed
wants to merge 8 commits into from
Closed

Remove old Room Directory #9056

wants to merge 8 commits into from

Conversation

justjanne
Copy link
Contributor

@justjanne justjanne commented Jul 14, 2022 •
edited by github-actions bot
Loading

Type: Task
Fixes: element-hq/element-web#22846

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

@justjanne justjanne added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Jul 14, 2022
@justjanne justjanne mentioned this pull request Jul 15, 2022
Merged
@justjanne justjanne force-pushed the justjanne/task/22846-remove-old-room-directory branch 2 times, most recently from 53f749b to f8809f3 Compare August 3, 2022 11:11
@justjanne justjanne force-pushed the justjanne/task/22846-remove-old-room-directory branch from 66fe255 to 2bc2d5c Compare August 8, 2022 11:17
@justjanne justjanne force-pushed the justjanne/task/22846-remove-old-room-directory branch from 2bc2d5c to 181d9fa Compare August 8, 2022 12:38
@justjanne justjanne force-pushed the justjanne/task/22846-remove-old-room-directory branch from 181d9fa to 4db8dc4 Compare August 11, 2022 10:59
@justjanne justjanne force-pushed the justjanne/task/22846-remove-old-room-directory branch from 4db8dc4 to a2eef46 Compare August 11, 2022 14:53
@justjanne justjanne force-pushed the justjanne/task/22846-remove-old-room-directory branch 2 times, most recently from abbb289 to 637adb1 Compare August 24, 2022 11:46
@justjanne justjanne force-pushed the justjanne/task/22846-remove-old-room-directory branch from 637adb1 to bf398f1 Compare August 24, 2022 13:00
@justjanne justjanne marked this pull request as ready for review August 24, 2022 13:27
@justjanne justjanne requested a review from a team as a code owner August 24, 2022 13:27
@turt2live
Copy link
Member

this appears to be a fairly massive change for the app, so would appreciate explicit review from the product and/or design team.

@turt2live turt2live requested a review from a team August 24, 2022 19:18
t3chguy
t3chguy reviewed Aug 25, 2022
View reviewed changes
cypress/e2e/room-directory/room-directory.spec.ts
@@ -72,32 +72,4 @@ describe("Room Directory", () => {
expect(resp.chunk[0].room_id).to.equal(roomId);
});
});

it("should allow finding published rooms in directory", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Is this case covered in the spotlight tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finding public rooms is in spotlight's tests.

But sure, I could add a test that creates a private room, publishes it via the UI, and then tries to find it, just for completeness' sake.

src/components/structures/RoomSearch.tsx Outdated Show resolved Hide resolved
@kittykat kittykat self-assigned this Sep 9, 2022
@kittykat
Copy link
Contributor

Are there any visual changes associated with this PR? Any risk of any remaining functionality breaking?

@turt2live
Copy link
Member

The UI change would be the loss of the public room directory button (from the home view). It's less of a risk than what it used to be, but also still fairly significant.

@kittykat
Copy link
Contributor

Changing the home screen is a more complicated issue which needs product thinking. Can that button link to this view instead of the old one?

Screenshot from 2022-10-17 22-11-06

@t3chguy
Copy link
Member

t3chguy commented Oct 18, 2022 •
edited
Loading

@kittykat that is already what happens, all interactions which went to the dialog being removed are re-routed to Spotlight-in-the-explore-state

image
image
image
image

t3chguy
t3chguy suggested changes Oct 18, 2022
View reviewed changes
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane but CI is generally unhappy with it

kittykat
kittykat approved these changes Oct 19, 2022
View reviewed changes
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 fine from product pov as all links to the directory have been updated

@dbkr dbkr removed their request for review November 3, 2022 17:02
@germain-gg
Copy link
Contributor

Superseeded by #9605

@germain-gg germain-gg closed this Nov 22, 2022
@justjanne justjanne deleted the justjanne/task/22846-remove-old-room-directory branch May 11, 2023 10:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@t3chguy t3chguy t3chguy requested changes

@kittykat kittykat kittykat approved these changes

@andybalaam andybalaam Awaiting requested review from andybalaam andybalaam is a code owner automatically assigned from matrix-org/element-web-reviewers

@duxovni duxovni Awaiting requested review from duxovni duxovni is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees

@kittykat kittykat

Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consolidate explore public rooms experiences
5 participants
@justjanne @turt2live @kittykat @t3chguy @germain-gg