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

Added TextInputWithCheckbox dialog #868

Merged

Conversation

psaavedra
Copy link
Contributor

@psaavedra psaavedra commented May 8, 2017

This change allows change the default value for m.federate value during the create action. Useful for enviroments where you prefer m.federate = False as default choice for the created rooms (see related PR).

This PR is embraces the following SPEC: https://docs.google.com/document/d/14zqsbwl5KKil-bB8w2HMhidBVmFkP9Q7EQKFwKIIfZc/edit#heading=h.eipip5qhqo0d

Other PR in synapse and matrix-js-sdk and riot-web are envolved

Signed-off-by: Pablo Saavedra psaavedra@igalia.com

Related PR:

Edited by Michael to match what the PR actually entails rather than the entire movement that it began as

@aperezdc
Copy link

aperezdc commented May 8, 2017

It's going to be great for people running their own home servers to have this setting available in the UI 💪

One suggestion/idea: Probably it would make sense to also add an element in the room settings panel which shows the status of the m.federate flag as part of this set of PRs (I am not sure whether that would go here or as part of element-hq/element-web#3849 though). Without something like that there is no way of knowing whether a room was created as federatable or not without checking with the HTTP API manually, right?

@psaavedra
Copy link
Contributor Author

psaavedra commented May 8, 2017 via email

@t3chguy
Copy link
Member

t3chguy commented Jun 30, 2017

would fix element-hq/element-web#1669

@t3chguy
Copy link
Member

t3chguy commented Aug 2, 2017

@aperezdc riot-web Room Settings already shows a message if m.federate=false

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Copy link
Member

t3chguy commented Aug 9, 2017

From conversation in riot-dev, needs an Advanced Settings accordion/spoiler thing

@t3chguy
Copy link
Member

t3chguy commented Aug 17, 2017

I'm thinking that the advanced option should mention m.federate explicitly as its wording is somewhat ambiguous

@ara4n
Copy link
Member

ara4n commented Oct 4, 2017

sorry that this has been stuck for so long, and huge thanks for submitting it (and @t3chguy for pushing it through). part of the stuckness has been due to confusion on whether this actually means to check the synapse API or not (and whether that API shape is okay). However, given it just goes off the riot config for now, the concern was confused.

However, the tests are failing due to:

/home/travis/build/matrix-org/matrix-react-sdk/src/components/views/dialogs/CreateRoomDialog.js
  51:53   error  A space is required after '{'               react/jsx-curly-spacing
  51:80   error  A space is required before '}'              react/jsx-curly-spacing
  54:154  error  A space is required before closing bracket  react/jsx-tag-spacing
  56:24   error  A space is required before closing bracket  react/jsx-tag-spacing
  61:120  error  A space is required before closing bracket  react/jsx-tag-spacing
  63:33   error  A space is required after '{'               react/jsx-curly-spacing
  63:102  error  A space is required before '}'              react/jsx-curly-spacing
  64:36   error  A space is required before closing bracket  react/jsx-tag-spacing
  65:34   error  A space is required after '{'               react/jsx-curly-spacing
  65:78   error  A space is required before '}'              react/jsx-curly-spacing

If fixed, i'll merge. thanks again.

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@aperezdc
Copy link

aperezdc commented Oct 5, 2017

One more data point supporting the merge of this PR: Our self hosted Riot instance has had this patch set since applied to it by @psaavedra to all Riot releases since last May, and it has been working well. That is a few months of testing 😉

@t3chguy
Copy link
Member

t3chguy commented Oct 9, 2017

@ara4n PTAL

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Oct 11, 2017

LGTM (given @ara4n was OK to merge pending linting)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants