Move manage integrations button from settings page to room header as a stand-alone component #1286

rxl881 · 2017-08-09T13:34:37Z

The intent of this PR is to move the existing manage integrations button out from the settings page in to a reusable component, so that it can be used in the room header (and other places, as needed).

into rxl881/appFixes

lukebarnard1 · 2017-08-11T09:30:20Z

src/components/views/elements/ManageIntegsButton.js

@@ -0,0 +1,127 @@
+/*
+Copyright 2017 Vector Creations Ltd


Not anymore!

lukebarnard1 · 2017-08-11T09:42:19Z

src/components/views/elements/ManageIntegsButton.js

+        super(props);
+
+        this.state = {
+            scalar_error: null,


Should be camelCase

lukebarnard1 · 2017-08-11T09:43:05Z

src/components/views/elements/ManageIntegsButton.js

+        let integrationsError;
+        if (this.scalarClient !== null) {
+            if (this.state.showIntegrationsError && this.state.scalar_error) {
+                console.error(this.state.scalar_error);


This should probably be logged after line 50

lukebarnard1 · 2017-08-11T09:46:13Z

src/components/views/elements/ManageIntegsButton.js

+        ScalarMessaging.startListening();
+        this.scalarClient = null;
+
+        if (SdkConfig.get().integrations_ui_url && SdkConfig.get().integrations_rest_url) {


It's strange that we don't pass these URLs into the auth client here, and have to check the config before creating a ScakarAuthClient.

Not an argument against fixing it, but this is simply copied across from the existing code on the settings page -- https://github.com/matrix-org/matrix-react-sdk/pull/1286/files#diff-7810353d2ee5218b7242e81423030433L141.

Unless you think this is broken, I'm inclined to leave it. What do you reckon?

It's not broken, it's just a weird API that made sense when we only ever made one Scalar client. Feel free to leave it for now.

Cool. Cheers.

I wrote this check to hide all instances of Manage Integrations/Scalar/Modular if there is are no integrations URLs defined in the config. i.e a way to hide the buttons.

This seems to have regressed from the original function: #816

lukebarnard1 · 2017-08-11T09:48:29Z

src/components/views/elements/ManageIntegsButton.js

+        Modal.createDialog(IntegrationsManager, {
+            src: (this.scalarClient !== null && this.scalarClient.hasCredentials()) ?
+                this.scalarClient.getScalarInterfaceUrlForRoom(this.props.room.roomId) :
+                null,


Would this not open an iFrame with a null src?

As with some of the previous comments, this has just been cargo-culted across from the existing room settings code (https://github.com/matrix-org/matrix-react-sdk/pull/1286/files#diff-7810353d2ee5218b7242e81423030433L520). As it is not currently considered broken, I'm inclined to leave it as is, for now.

lukebarnard1 · 2017-08-11T09:50:20Z

src/components/views/elements/ManageIntegsButton.js

+}
+
+ManageIntegsButton.propTypes = {
+    room: PropTypes.object.isRequired,


I'd prefer a roomId going in here, ManageIntegsButton doesn't need the rest of room state.

lukebarnard1 · 2017-08-11T09:51:16Z

src/components/views/elements/ManageIntegsButton.js

+                null,
+            onFinished: ()=>{
+                if (this.props.onCancelClick) {
+                    this.props.onCancelClick(ev);


You probably don't want to be returning this ev here

As with one of the previous comments, this code is directly copied across from the existing settings page - https://github.com/matrix-org/matrix-react-sdk/pull/1286/files#diff-7810353d2ee5218b7242e81423030433L525.

I'm inclined to leave it, for fear of breaking existing expected behaviour.

lukebarnard1 · 2017-08-11T09:55:24Z

src/components/views/rooms/RoomHeader.js

        if (!this.props.editing) {
            rightRow =
                <div className="mx_RoomHeader_rightRow">
                    { settingsButton }
+                    <ManageIntegsButton
+                        onCancelClick={this.props.onCancelClick}


It seems quite bizarre to be bubbling the onCancelClick of the integrations manager to the onCancelClick of the room header. Stranger still that the onCancelClick that is passed to RoomView will close room settings, so this is definitely wrong.

Thanks for spotting that!

Show a dialog if the maximum number of widgets allowed has been reached.

rxl881 · 2017-08-17T11:23:17Z

Thanks Luke. Hopefully that is mainly sorted now. There are two items that were copied across from existing code and I'm tempted to leave, unless you think that they should be addressed now. Can you PTAL?

Cheers.

into rxl881/appFixes

lukebarnard1 · 2017-08-18T09:03:04Z

src/components/views/elements/ManageIntegsButton.js

+        ScalarMessaging.startListening();
+        this.scalarClient = null;
+
+        if (SdkConfig.get().integrations_ui_url && SdkConfig.get().integrations_rest_url) {


It's not broken, it's just a weird API that made sense when we only ever made one Scalar client. Feel free to leave it for now.

lukebarnard1 · 2017-08-18T09:07:19Z

src/components/views/elements/ManageIntegsButton.js

+                null,
+            onFinished: ()=>{
+                if (this.props.onCancelClick) {
+                    this.props.onCancelClick(ev);


for fear of breaking existing expected behaviour.

This property is never set - the entire if is redundant and we should absolutely avoid redundant, potentially confusing code.

This means that the behaviour has changed, given you've copied it. Oh! I guess this is the thing where if you close the integrations manager, you also close the room settings. Maybe that wasn't so bizarre after all?

As discussed IRL, I've removed this all together from the ManageIntegsButton component, as it doesn't appear to be used at the moment.

lukebarnard1 · 2017-08-18T09:09:46Z

src/components/views/elements/ManageIntegsButton.js

+        let integrationsError;
+        if (this.scalarClient !== null) {
+            if (this.state.showIntegrationsError && this.state.scalarError) {
+                console.error(this.state.scalarError);


I suggest moving this to where the error is first caught.

lukebarnard1 · 2017-08-18T09:13:21Z

src/components/views/rooms/RoomHeader.js

@@ -47,13 +48,15 @@ module.exports = React.createClass({
        onSaveClick: React.PropTypes.func,
        onSearchClick: React.PropTypes.func,
        onLeaveClick: React.PropTypes.func,
+        onCancelClick: React.PropTypes.func,


onCancelClick is not used by anything

Although it is not used by anything that I have added, this prop actually is used elsewhere in the code... https://github.com/matrix-org/matrix-react-sdk/pull/1286/files#diff-96b79748dd7ebec5d36339d2478e92cbR194

I was adding it in to the component props and default to make it clear to future people (like me) that this prop is passed in are available to be used by other components and to make sure that there is a sensible default if the function is not passed.

I can remove it if you want, but I think it makes more sense to leave it?

My mistake, this looks to be necessary. I think I was confused by the fact that it wasn't in prop types previously.

lukebarnard1 · 2017-08-18T11:53:50Z

src/components/views/elements/ManageIntegsButton.js

+    roomId: PropTypes.string.isRequired,
+};
+
+ManageIntegsButton.defaultProps = {


Empty default props

lukebarnard1 · 2017-08-18T11:57:03Z

src/components/views/rooms/RoomHeader.js

@@ -47,13 +48,15 @@ module.exports = React.createClass({
        onSaveClick: React.PropTypes.func,
        onSearchClick: React.PropTypes.func,
        onLeaveClick: React.PropTypes.func,
+        onCancelClick: React.PropTypes.func,


My mistake, this looks to be necessary. I think I was confused by the fact that it wasn't in prop types previously.

into rxl881/appFixes

rxl881 added 7 commits August 5, 2017 00:00

Switch app drawer icons

1973b2b

Move room settings button to RoomHeader

a22e768

CancelClick prop.

308d932

Send messages on widget addition and deletion

18ae5fd

Split timeline updates in to different PR.

9f8e8ae

Move manage integrations button in to stand-alone component

4bc25f1

Merge branch 'develop' of https://github.com/matrix-org/matrix-react-sdk

185379b

into rxl881/appFixes

rxl881 requested a review from dbkr August 9, 2017 13:34

Show a dialog if the maximum number of widgets allowed has been reached.

0323151

rxl881 mentioned this pull request Aug 10, 2017

Show a dialog if the maximum number of widgets allowed has been reached. #1291

Merged

rxl881 requested a review from lukebarnard1 August 11, 2017 09:07

lukebarnard1 self-assigned this Aug 11, 2017

Merge branch 'develop' of https://github.com/matrix-org/matrix-react-sdk

454ec40

into rxl881/appFixes

lukebarnard1 suggested changes Aug 11, 2017

View reviewed changes

lukebarnard1 assigned rxl881 and unassigned lukebarnard1 Aug 11, 2017

rxl881 and others added 5 commits August 17, 2017 10:03

Merge pull request #1291 from matrix-org/rxl881/maxWidgets

9111cb4

Show a dialog if the maximum number of widgets allowed has been reached.

Fix copyright header

2c25639

Camel case variable name

eb77dcc

Pass roomId rather than whole room object.

d1ee257

Don't bubble cancel click to room header.

02edadb

t3chguy assigned lukebarnard1 and unassigned rxl881 Aug 17, 2017

rxl881 added 2 commits August 17, 2017 17:47

Merge branch 'develop' of https://github.com/matrix-org/matrix-react-sdk

0907fff

into rxl881/appFixes

Check for valid roomId before renering manageIntegsButton.

120a4f4

rxl881 requested a review from ara4n August 17, 2017 17:18

Kick travis

95d1c37

lukebarnard1 suggested changes Aug 18, 2017

View reviewed changes

Move error logging to where it is first caught.

96900e7

REmove unused onCancelClick

84f5e5a

lukebarnard1 suggested changes Aug 18, 2017

View reviewed changes

lukebarnard1 assigned rxl881 and unassigned lukebarnard1 Aug 18, 2017

Remove empty defaultProps.

857a8c9

lukebarnard1 approved these changes Aug 18, 2017

View reviewed changes

Merge branch 'develop' of https://github.com/matrix-org/matrix-react-sdk

b7d46d9

into rxl881/appFixes

lukebarnard1 approved these changes Aug 18, 2017

View reviewed changes

Fix add widget link

1c36e47

rxl881 merged commit 942656d into develop Aug 18, 2017

rxl881 deleted the rxl881/appFixes branch August 18, 2017 15:09

t3chguy mentioned this pull request Aug 23, 2017

regression with new Manage Integrations button hiding element-hq/element-web#4852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move manage integrations button from settings page to room header as a stand-alone component #1286

Move manage integrations button from settings page to room header as a stand-alone component #1286

rxl881 commented Aug 9, 2017 •

edited

Loading

lukebarnard1 Aug 11, 2017

lukebarnard1 Aug 11, 2017

lukebarnard1 Aug 11, 2017

lukebarnard1 Aug 11, 2017

rxl881 Aug 17, 2017

lukebarnard1 Aug 18, 2017

rxl881 Aug 18, 2017

t3chguy Aug 23, 2017

lukebarnard1 Aug 11, 2017

rxl881 Aug 17, 2017

lukebarnard1 Aug 11, 2017

rxl881 Aug 17, 2017

lukebarnard1 Aug 11, 2017

rxl881 Aug 17, 2017

lukebarnard1 Aug 11, 2017

rxl881 Aug 17, 2017

rxl881 commented Aug 17, 2017

lukebarnard1 Aug 18, 2017

lukebarnard1 Aug 18, 2017

rxl881 Aug 18, 2017

lukebarnard1 Aug 18, 2017

rxl881 Aug 18, 2017

lukebarnard1 Aug 18, 2017

rxl881 Aug 18, 2017

lukebarnard1 Aug 18, 2017

lukebarnard1 Aug 18, 2017

lukebarnard1 Aug 18, 2017

Move manage integrations button from settings page to room header as a stand-alone component #1286

Move manage integrations button from settings page to room header as a stand-alone component #1286

Conversation

rxl881 commented Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxl881 commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxl881 commented Aug 9, 2017 •

edited

Loading