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

Conversation

charlynguyen
Copy link
Contributor

@charlynguyen charlynguyen commented Aug 23, 2023

We @nordeck are currently implementing the knock rooms behind the feature flag feature_ask_to_join (introduced in #11182).

This pull request allows setting knock room visibility.

  • Add checkbox to handle the knock room visibility when creating rooms (CreateRoomDialog).
  • Add checkbox to handle the knock room visibility when editing rooms (JoinRuleSettings).
  • Introduce "Ask to join" button to spotlight options to directly prompt the request to join (SpotlightDialog).

Epic: element-hq/element-web#18655

Is blocked by

Relates to

Screens / Videos

CreateRoomDialog

CreateRoomDialog

JoinRuleSettings

JoinRuleSettings

SpotlightDialog

SpotlightDialog

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements labels Aug 23, 2023
Signed-off-by: Charly Nguyen <charly.nguyen@nordeck.net>
@charlynguyen charlynguyen force-pushed the charlynguyen/allow-setting-knock-room-visibility branch from cae8b82 to 798f13a Compare August 23, 2023 14:52
@charlynguyen charlynguyen marked this pull request as ready for review August 23, 2023 15:30
@charlynguyen charlynguyen requested a review from a team as a code owner August 23, 2023 15:30
@charlynguyen
Copy link
Contributor Author

FYI @daniellekirkwood 😄

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.

Thank you for your contribution @charlynguyen . Please have a look at my comments:

We are using switches in most places. Can you replace the checkboxes with switches? Otherwise the UI would be (more) inconsistent. Update: Looks like design suggests it this way.

switch checkbox
image image

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Some additional thoughts from me:

  • This feels like three separate changes, which are only very tangentially related. I think it would be easier to review and merge if split into separate PRs.
  • We already have a switch to control whether a room is in the public room directory (on the "General" tab) - I think adding a second control on another tab will be confusing.

@@ -298,6 +304,18 @@ export default class CreateRoomDialog extends React.Component<IProps, IState> {
);
}

let visibilitySection: JSX.Element | undefined;
if (this.state.joinRule === JoinRule.Knock) {
Copy link
Member

Choose a reason for hiding this comment

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

why do we only expose this control for knockable rooms? Isn't it relevant to other rooms too?

@@ -215,6 +217,10 @@ export default class CreateRoomDialog extends React.Component<IProps, IState> {
return result;
};

private onIsPublicChange = (isPublic: boolean): void => {
this.setState({ isPublic });
Copy link
Member

Choose a reason for hiding this comment

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

for reference: I was confused about what this isPublic state was doing. The answer is: nothing. It appears to have been introduced by #5698, probably due to an incorrect merge resolution due to a conflict with #6212 (which removed it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was also my conclusion. So, I decided to give it a purpose.

@weeman1337
Copy link
Contributor

* We already have a switch to control whether a room is in the public room directory (on the "General" tab) - I think adding a second control on another tab will be confusing.

I've checked the designs. This is how they suggested it. Doesn't mean we shouldn't open this up for discussion.

@richvdh
Copy link
Member

richvdh commented Aug 24, 2023

I've checked the designs. This is how they suggested it. Doesn't mean we shouldn't open this up for discussion.

yeah... I'd like to understand the reasoning behind that decision. Duplicating the setting, but only for knockable rooms, seems confusing to me.

@daniellekirkwood
Copy link

I agree with this:

We already have a switch to control whether a room is in the public room directory (on the "General" tab) - I think adding a second control on another tab will be confusing.

However, the switch being "point of need" at the "ask to join" part of settings makes sense to me. If we want to remove it from General, then maybe we explore that.

Having discussed the "Compoundifying" of Knocking with @nadonomy this morning we've agreed that it's not on our critical path and we're happy with the team at Nordeck to push forwards with the Knocking designs that we have provided them - then agreeing that when we get to Settings in the Compound project (Q4/Q1) we will review all the switches, toggles, pages and repeated info hollistically and remove them later.

@richvdh
Copy link
Member

richvdh commented Aug 24, 2023

Having discussed the "Compoundifying" of Knocking with @nadonomy this morning we've agreed that it's not on our critical path and we're happy with the team at Nordeck to push forwards with the Knocking designs that we have provided them

@daniellekirkwood can you just confirm my understanding on a few points:

  • You're happy for these controls to be checkboxes rather than switches
  • You're happy to duplicate the "include in public directory" switch to multiple room-settings tabs
  • You're happy for the setting to only appear for knockable rooms, and not (say) "public" rooms. (This seems particularly unintuitive to me)

@daniellekirkwood
Copy link

You're happy for these controls to be checkboxes rather than switches

I'm happy for this PR to use the method that's been shared in the designs. Esp. knowing that it's likely we'll update it in the next 12 months as part of the compound project. Unless @nadonomy has a big blocker here.

You're happy to duplicate the "include in public directory" switch to multiple room-settings tabs

Not happy but believe it's a temporary necessity as we don't have the answer to where it should like (nor the time/bandwidth to find the answer). The two switches should be linked though - if one is on, the other should reflect that state and vice versa.

You're happy for the setting to only appear for knockable rooms, and not (say) "public" rooms. (This seems particularly unintuitive to me)

Good point -- when someone selects Public it doesn't necessarily add it to the directory, right? Maybe this checkbox should appear there also... @nadonomy i would like support on that question...

@charlynguyen
Copy link
Contributor Author

Thank you for your contribution @charlynguyen . Please have a look at my comments:

We are using switches in most places. Can you replace the checkboxes with switches? Otherwise the UI would be (more) inconsistent. Update: Looks like design suggests it this way.

switch checkbox
image image

That was also my impression during the implementation, but I decided to stick to the screens.

@richvdh
Copy link
Member

richvdh commented Aug 29, 2023

It seems like this does match the intentions from the design team, but I'd still rather see it land as three separate pull requests rather than a single one. @charlynguyen would you mind breaking it up?

@charlynguyen
Copy link
Contributor Author

It seems like this does match the intentions from the design team, but I'd still rather see it land as three separate pull requests rather than a single one. @charlynguyen would you mind breaking it up?

That is totally fine by me 🙂

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