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

Remove "Add Space" button in RoomListHeader when user cannot createSpaces #9002

Conversation

estellecomment
Copy link
Contributor

@estellecomment estellecomment commented Jul 7, 2022

Problem description :
When using ComponentVilisbility to disable UIComponent.CreateSpaces, buttons for creating spaces should be hidden.

But there is one that is still displayed, in RoomListHeader, for creating a subspace :
Screen Shot 2022-07-07 at 10 29 08

This is incoherent with SpaceContextMenu, where you can also create a subspace, which does hide the button (as expected) :
176678282-c35fff4c-d561-4e79-9d0b-d3ed12f0b5a8

Expected : the button for adding subspaces is hidden everywhere.
Obtained : one button for adding subspaces is hidden (in SpaceContextMenu), the other is shown (in RoomListHeader).
Proposed fix : hide the button for adding subspaces in RoomListHeader. Reuse similar code as SpaceContextMenu to harmonize.

Notes for reviewers : in order to see the fix in element-web, you need to use ComponentVisibility to get shouldShowComponent(UIComponent.CreateSpaces) == false.
Example PR to do this, in a fork of element-web : https://github.com/tchapgouv/element-web/pull/2/files

Signed-off-by: Estelle Comment estelle.comment@gmail.com


Here's what your changelog entry will look like:

✨ Features

  • Remove "Add Space" button in RoomListHeader when user cannot createSpaces (#9002). Contributed by @estellecomment.

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Jul 7, 2022
@estellecomment
Copy link
Contributor Author

@weeman1337 who follows this issue 👋

@weeman1337 weeman1337 self-requested a review July 7, 2022 08:55
@weeman1337 weeman1337 added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Jul 7, 2022
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

It would be good to have a test covering the changes.

There is already a RoomListHeader test that can be extended: https://github.com/matrix-org/matrix-react-sdk/blob/develop/test/components/views/rooms/RoomListHeader-test.tsx

If you need help on this just send me a message.

src/components/views/rooms/RoomListHeader.tsx Outdated Show resolved Hide resolved
@estellecomment
Copy link
Contributor Author

@weeman1337 Done with tests, ready for re-review !

Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

Thanks for following through with the tests, this looks really good! My comments are mostly just about types, and places where we have a more modern way of doing things.

test/components/views/rooms/RoomListHeader-test.tsx Outdated Show resolved Hide resolved
test/components/views/rooms/RoomListHeader-test.tsx Outdated Show resolved Hide resolved
test/components/views/rooms/RoomListHeader-test.tsx Outdated Show resolved Hide resolved
test/components/views/rooms/RoomListHeader-test.tsx Outdated Show resolved Hide resolved
test/components/views/rooms/RoomListHeader-test.tsx Outdated Show resolved Hide resolved
test/components/views/rooms/RoomListHeader-test.tsx Outdated Show resolved Hide resolved
test/components/views/rooms/RoomListHeader-test.tsx Outdated Show resolved Hide resolved
@mcalinghee mcalinghee force-pushed the estellecomment/remove-add-space-button branch from eb4d917 to ffc0ea9 Compare July 29, 2022 12:55
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

The loading-test suite appears to be failing

@mcalinghee mcalinghee force-pushed the estellecomment/remove-add-space-button branch from e8d9182 to 533fb31 Compare July 29, 2022 15:19
@mcalinghee
Copy link
Contributor

The loading-test suite appears to be failing

This is now fixed

@robintown
Copy link
Member

Great, could you please merge develop to restart CI?

@robintown
Copy link
Member

Also, for future reference, please don't force push to a branch after it's been reviewed, as that makes it more difficult to tell what changes were reviewed and which weren't.

Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

We're still trying to get our CI to work but everything looks good to go in theory

@robintown
Copy link
Member

Our CI is currently unable to test this change, as it's matching with a branch of the same name in the Tchap element-web fork which disables space creation entirely. @estellecomment or @mcalinghee, you'll need to rename either this branch or the element-web branch in your forks, so that they no longer get matched.

@weeman1337
Copy link
Contributor

Thanks for updating everything 👍 I will trigger the builds again.

@mcalinghee
Copy link
Contributor

Our CI is currently unable to test this change, as it's matching with a branch of the same name in the Tchap element-web fork which disables space creation entirely. @estellecomment or @mcalinghee, you'll need to rename either this branch or the element-web branch in your forks, so that they no longer get matched.

Thanks a lot, this is now done

@robintown
Copy link
Member

One of our CI checks wasn't able to run because of this PR being a month old, so I've opened a new PR with the same changes.

@robintown robintown closed this Aug 3, 2022
auto-merge was automatically disabled August 3, 2022 15:01

Pull request was closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants