Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added TextInputWithCheckbox dialog #868

Merged
merged 10 commits into from Oct 11, 2017

Conversation

Projects
None yet
5 participants
Contributor

psaavedra commented May 8, 2017 edited by t3chguy

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 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 vector-im/riot-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?

Contributor

psaavedra commented May 8, 2017

Collaborator

t3chguy commented Jun 30, 2017

Collaborator

t3chguy commented Aug 2, 2017

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

t3chguy added some commits Aug 2, 2017

fix and i18n the impl
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
fix m.federate=false warning in room settings
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
fix m.federate always being false because value is a string. Y HTML...
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Collaborator

t3chguy commented Aug 9, 2017

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

t3chguy added some commits Aug 17, 2017

Merge remote-tracking branch 'remotes/origin/develop' into set_defaul…
…t_federate_by_settings

# Conflicts:
#	src/components/structures/MatrixChat.js
Collaborator

t3chguy commented Aug 17, 2017

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

Owner

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.

t3chguy added some commits Oct 5, 2017

delint
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
remove redundant&stale onKeyDown
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

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 😉

Collaborator

t3chguy commented Oct 9, 2017

@ara4n PTAL

Contributor

lukebarnard1 commented Oct 11, 2017 edited

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

@lukebarnard1 lukebarnard1 merged commit ffb9dd8 into matrix-org:develop Oct 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment