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

rework roomsettings for new visibility UI #241

Merged
merged 3 commits into from
Mar 22, 2016
Merged

rework roomsettings for new visibility UI #241

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cfb0e9
rework roomsettings for new visibility UI
ara4n Mar 22, 2016
df905cf
incorporate PR review, and explicitly spell out if the room has ended…
ara4n Mar 22, 2016
5a2a2c5
fix up and factor out mayChangeRoomAccess and fix review feedback
ara4n Mar 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
rework roomsettings for new visibility UI
  • Loading branch information
@ara4n
ara4n committed Mar 22, 2016
commit 8cfb0e9ef45436a4f2981a2f2d01507dd3911239
123 changes: 101 additions & 22 deletions src/components/views/rooms/RoomSettings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,20 @@ module.exports = React.createClass({
power_levels_changed: false,
tags_changed: false,
tags: tags,
areNotifsMuted: areNotifsMuted
areNotifsMuted: areNotifsMuted,
Copy link
Member

Choose a reason for hiding this comment

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

It would be polite to put isRoomPublished here, so we don't have to read the entire file to find out what our state vars are :)

};
},

componentWillMount: function() {
var self = this;
MatrixClientPeg.get().getRoomVisibility(
this.props.room.roomId
).done((result) => {
self.setState({ isRoomPublished: result.visibility === "public" });
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You're being awesome and using arrow functions, but yet still using self. Arrow functions capture this correctly, so you can drop the self.

}, (err) => {
console.error("Failed to get room visibility: " + err);
});
},

setName: function(name) {
this.setState({
Expand Down Expand Up @@ -112,6 +123,13 @@ module.exports = React.createClass({
));
}

if (this.state.isRoomPublished !== originalState.isRoomPublished) {
promises.push(MatrixClientPeg.get().setRoomVisibility(
roomId,
this.state.isRoomPublished ? "public" : "private"
));
}

if (this.state.join_rule !== originalState.join_rule) {
promises.push(MatrixClientPeg.get().sendStateEvent(
roomId, "m.room.join_rules",
Expand Down Expand Up @@ -252,6 +270,30 @@ module.exports = React.createClass({
});
},

_onRoomAccessRadioToggle: function(ev) {
var self = this;
Copy link
Member

Choose a reason for hiding this comment

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

No closures are being used here at all; using self in this function is redundant.

switch (ev.target.value) {
case "invite_only":
self.setState({
join_rule: "invite",
guest_access: "can_join",
Copy link
Member

Choose a reason for hiding this comment

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

Really? I'd have thought forbidden...

Copy link
Member

Choose a reason for hiding this comment

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

I would love to see a 2x2 matrix of the options here:

                      join_rule
                  INVITE  |  PUBLIC
       -------------------+----------------
guest  CAN_JOIN | inv_only| pub_with_guest
access -------------------+----------------
       NO_JOIN  | inv_only| pub_no_guest
       -------------------+----------------

You can see my concern more clearly here: there's no way to actually set all 4 states with the present UI, and no option seems intuitive here I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

half the point of these new settings is to make sure that if a room is invite only, and you invite a guest, you don't have to remember to also set a 'guest access is true' checkbox. If you're inviting someone explicitly, you know you want them to join anyway whether they're a guest or not.

So yes, you can't set all 4 states, but this is very much deliberate. Will put the table in to spell it out.

Copy link
Member

Choose a reason for hiding this comment

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

Can you put your blurb about why we set can_join here please.

});
break;
case "public_no_guests":
self.setState({
join_rule: "public",
guest_access: "forbidden",
});
break;
case "public_with_guests":
self.setState({
join_rule: "public",
guest_access: "can_join",
});
break;
}
},

_onToggle: function(keyName, checkedValue, uncheckedValue, ev) {
console.log("Checkbox toggle: %s %s", keyName, ev.target.checked);
var state = {};
Expand Down Expand Up @@ -427,7 +469,20 @@ module.exports = React.createClass({
// http://matrix.org/docs/spec/r0.0.0/client_server.html#id31
var historyVisibility = this.state.history_visibility || "shared";

// FIXME: disable guests_read if the user hasn't turned on shared history
var addressWarning;
var aliasEvents = this.props.room.currentState.getStateEvents('m.room.aliases') || [];
var aliasCount = 0;
aliasEvents.forEach((event) => {
aliasCount += event.getContent().aliases.length;
});

if (this.state.join_rule === "public" && aliasCount == 0) {
addressWarning =
<div className="mx_RoomSettings_warning">
To link to a room it must have <a href="#addresses">an address</a>.
</div>
}

return (
<div className="mx_RoomSettings">

Expand All @@ -440,43 +495,65 @@ module.exports = React.createClass({
defaultChecked={this.state.areNotifsMuted}/>
Mute notifications for this room
</label>
<label>
<input type="checkbox" disabled={ !roomState.mayClientSendStateEvent("m.room.join_rules", cli) }
onChange={this._onToggle.bind(this, "join_rule", "invite", "public")}
defaultChecked={this.state.join_rule !== "public"}/>
Make this room private
</label>
<label>
<input type="checkbox" disabled={ !roomState.mayClientSendStateEvent("m.room.guest_access", cli) }
onChange={this._onToggle.bind(this, "guest_access", "can_join", "forbidden")}
defaultChecked={this.state.guest_access === "can_join"}/>
Let guests join this room
</label>
<div className="mx_RoomSettings_settings">
<h3>Who can access this room?</h3>
<label>
<input type="radio" name="roomVis" value="invite_only"
disabled={ !roomState.mayClientSendStateEvent("m.room.join_rules", cli) }
Copy link
Member

Choose a reason for hiding this comment

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

Technically this is a lie. If guest_access is forbidden then you need to flip it to be can_join, so you need to have permission to send m.room.guest_access too. (in which case, factor out the check please!)

onChange={this._onRoomAccessRadioToggle}
defaultChecked={this.state.join_rule !== "public"}/>
Only people who have been invited
</label>
<label>
<input type="radio" name="roomVis" value="public_no_guests"
disabled={ !(roomState.mayClientSendStateEvent("m.room.join_rules", cli) &&
roomState.mayClientSendStateEvent("m.room.guest_access", cli)) }
onChange={this._onRoomAccessRadioToggle}
defaultChecked={this.state.join_rule === "public" && this.state.guest_access !== "can_join"}/>
Anyone who knows the room's link, apart from guests
</label>
<label>
<input type="radio" name="roomVis" value="public_with_guests"
disabled={ !(roomState.mayClientSendStateEvent("m.room.join_rules", cli) &&
roomState.mayClientSendStateEvent("m.room.guest_access", cli)) }
onChange={this._onRoomAccessRadioToggle}
defaultChecked={this.state.join_rule === "public" && this.state.guest_access === "can_join"}/>
Anyone who knows the room's link, including guests
</label>
{ addressWarning }
<br/>
<label>
<input type="checkbox" disabled={ !roomState.mayClientSendStateEvent("m.room.aliases", cli) }
onChange={ this._onToggle.bind(this, "isRoomPublished", true, false)}
checked={this.state.isRoomPublished}/>
List this room in { MatrixClientPeg.get().getDomain() }'s directory?
</label>
</div>
<div className="mx_RoomSettings_settings">
<h3>Who can read history?</h3>
<label htmlFor="hvis_wr">
<input type="radio" id="hvis_wr" name="historyVis" value="world_readable"
<label>
<input type="radio" name="historyVis" value="world_readable"
disabled={ !roomState.mayClientSendStateEvent("m.room.history_visibility", cli) }
checked={historyVisibility === "world_readable"}
onChange={this._onHistoryRadioToggle} />
Anyone
</label>
<label htmlFor="hvis_sh">
<input type="radio" id="hvis_sh" name="historyVis" value="shared"
<label>
<input type="radio" name="historyVis" value="shared"
disabled={ !roomState.mayClientSendStateEvent("m.room.history_visibility", cli) }
checked={historyVisibility === "shared"}
onChange={this._onHistoryRadioToggle} />
Members only (since the point in time of selecting this option)
</label>
<label htmlFor="hvis_inv">
<input type="radio" id="hvis_inv" name="historyVis" value="invited"
<label>
<input type="radio" name="historyVis" value="invited"
disabled={ !roomState.mayClientSendStateEvent("m.room.history_visibility", cli) }
checked={historyVisibility === "invited"}
onChange={this._onHistoryRadioToggle} />
Members only (since they were invited)
</label>
<label htmlFor="hvis_joi">
<input type="radio" id="hvis_joi" name="historyVis" value="joined"
<label >
<input type="radio" name="historyVis" value="joined"
disabled={ !roomState.mayClientSendStateEvent("m.room.history_visibility", cli) }
checked={historyVisibility === "joined"}
onChange={this._onHistoryRadioToggle} />
Expand All @@ -495,6 +572,8 @@ module.exports = React.createClass({
<ColorSettings ref="color_settings" room={this.props.room} />
</div>

<a id="addresses"/>

<AliasSettings ref="alias_settings"
roomId={this.props.room.roomId}
canSetCanonicalAlias={ roomState.mayClientSendStateEvent("m.room.canonical_alias", cli) }
Expand Down