-
-
Notifications
You must be signed in to change notification settings - Fork 827
Remove "Add Space" button in RoomListHeader when user cannot createSpaces #9002
Remove "Add Space" button in RoomListHeader when user cannot createSpaces #9002
Conversation
@weeman1337 who follows this issue 👋 |
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.
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.
@weeman1337 Done with tests, ready for re-review ! |
a7da141
to
f0c5159
Compare
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.
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.
eb4d917
to
ffc0ea9
Compare
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.
The loading-test suite appears to be failing
e8d9182
to
533fb31
Compare
This is now fixed |
Great, could you please merge develop to restart CI? |
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. |
…ove-add-space-button
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.
We're still trying to get our CI to work but everything looks good to go in theory
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 for updating everything 👍 I will trigger the builds again. |
Thanks a lot, this is now done |
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. |
Pull request was closed
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](https://user-images.githubusercontent.com/911434/177731063-11c810eb-00ff-41fa-a36e-1c5be480a5ec.png)
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](https://user-images.githubusercontent.com/911434/177730072-e634a8ea-36c9-48fb-a2da-47cea2f225be.png)
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 [email protected]
Here's what your changelog entry will look like:
✨ Features