Improve widget rendering on prop updates #1548

rxl881 · 2017-10-27T16:52:56Z

Taking a different approach to #1542.

lukebarnard1 · 2017-10-30T16:56:18Z

src/components/views/elements/AppTile.js

+        this.updateWidgetContent();
+    },
+
+    // Update widget content


Redundant comment

lukebarnard1 · 2017-10-30T16:57:08Z

src/components/views/elements/AppTile.js

+
+    // Update widget content
+    updateWidgetContent() {
+        this.setState(this._getInitialState());


The name "getInitialState" isn't really accurate if the state that it returns is going to be updating.

Fair enough. I've renamed it to "_getNewUrlState".

lukebarnard1 · 2017-10-30T16:58:00Z

src/components/views/elements/AppTile.js

    },

    componentWillUnmount() {
        window.removeEventListener('message', this._onMessage);
    },

+    componentDidUpdate(prevProps) {


I think you want to do this before an update happens, i.e. using https://reactjs.org/docs/react-component.html#componentwillreceiveprops

This was intentional, so that the _getInitialState function (now renamed to _getNewUrlState) could use the current properties, whether called from the initial load (getInitialState), or when the properties have been updated. By switching it around we have to pass the properties to be used ... which seems clunkier to me, and needs a comment to explain why we are passing props to the function, rather than just using "this.props".

Anyway, I've gone and changed it around, as suggested...

lukebarnard1 · 2017-10-30T17:01:21Z

src/components/views/elements/AppTile.js

@@ -333,6 +359,7 @@ export default React.createClass({
                            alt={_t('Edit')}
                            title={_t('Edit')}
                            onClick={this._onEditClick}
+                            onLoad={this._onLoaded}


This seems like a bizarre thing to be doing. I don't think this image's rendering has anything to do with the loading state of scalar.

Meh... you're quite right, this should be on the iframe. Thanks for catching that!

lukebarnard1 · 2017-10-30T17:02:00Z

src/components/views/elements/AppTile.js

-            loading: true,
-        });
-        this._scalarClient = new ScalarAuthClient();
+        if(! this._scalarClient) {


spurious space

lukebarnard1 · 2017-10-30T17:03:06Z

src/components/views/elements/AppTile.js

        if (!this.isScalarUrl()) {
            return;
        }
        // Fetch the token before loading the iframe as we need to mangle the URL
-        this.setState({
-            loading: true,


If this is no longer needed, the partner loading: falses should be removed too. But it would be strange to not display this loading state, no? Unless this happens too quickly for a spinner to be worth it?

The loading state is now set to true in the _getInitialState function on line 64/65. The loading state spinner is shown until the onload event of the iframe is called. Leaving this as is.

lukebarnard1 · 2017-10-30T17:06:17Z

src/components/views/elements/AppTile.js

-                u.search = "?scalar_token=" + encodeURIComponent(token);
-            } else {
-                u.search += "&scalar_token=" + encodeURIComponent(token);
+            const params = new URLSearchParams(u.search);


Looks like we already have a library for this - riot-web uses querystring (looking at url_utils.js)... although it doesn't seem to be marked as a dependency.

Urghhh... undocumented dependencies. Yay. I'll switch to using that then. I'll also add querystring in to the package.json to make it more obvious that this library is available.

into rxl881/widgetrendering

rxl881 · 2017-11-07T14:44:02Z

@lukebarnard1 -- Sorry, this one got lost / delayed. Can you please take a quick look (again) and let me know if it is good to go?

Thanks

lukebarnard1 · 2017-11-08T10:12:16Z

package.json

@@ -74,6 +74,8 @@
    "matrix-js-sdk": "0.8.5",
    "optimist": "^0.6.1",
    "prop-types": "^15.5.8",
+    "qs": "^6.5.1",
+    "querystring": "^0.2.0",


I don't think this is used.

lukebarnard1 · 2017-11-08T10:17:45Z

src/components/views/elements/AppTile.js

-        const widgetPermissionId = [this.props.room.roomId, encodeURIComponent(this.props.url)].join('_');
+    /**
+     * Set initial component state when the App wUrl (widget URL) is being updated
+     * @param  {Object} props The component props *must* be passed (rather than using this.props) so that it can be called with future props, e.g. from componentWillReceiveProps.


This doc seems to be explaining why props is here instead of what it is. Better to say what it is (i.e. newProps - the new properties of the component). The explanation could go in the description for the function itself.

lukebarnard1 · 2017-11-08T16:56:45Z

src/components/views/elements/AppTile.js

        const hasPermissionToLoad = localStorage.getItem(widgetPermissionId);
        return {
-            loading: false,
-            widgetUrl: this.props.url,
+            initialising: true,   // True while we are mangling the widget URL


ideally, this would be set to true in the function that also sets it to false, then the explanation isn't needed

Both the initialising and loading flags need to be set as initial state (before other functions are called). Normally this would be set in getInitialState. However, as this code is reused to reset state at other points in the lifecycle (on URL change), I've pulled it out in to a separate function that is called from getInitialState. Unless you have major concerns, I'll leave this as is.

It's just nicer to read, instead of wondering "oh why did loading get set to false here?... where did it get to true... ah way over here"

Sure, that makes sense. However, given the reason for doing it like this (as described in the comment above), do you still think that changes are needed? If so, I'm not understanding the change that you are suggesting (sorry for the confusion).

We've discussed this out of band and (hopefully) cleared up the confusion on this one. I have changed it to explicitly set the state to 'initialising: true' in the setScalarToken function.

lukebarnard1 · 2017-11-08T16:58:49Z

src/components/views/elements/AppTile.js

-            loading: false,
-            widgetUrl: this.props.url,
+            initialising: true,   // True while we are mangling the widget URL
+            loading: true,        // True while the iframe content is loading


likewise for this one

As discussed, leaving this for the time being. However, may need a refactor for improved clarity in future.

lukebarnard1 · 2017-11-08T17:00:19Z

src/components/views/elements/AppTile.js

            widgetPermissionId: widgetPermissionId,
            // Assume that widget has permission to load if we are the user who added it to the room, or if explicitly granted by the user
-            hasPermissionToLoad: hasPermissionToLoad === 'true' || this.props.userId === this.props.creatorUserId,
+            hasPermissionToLoad: hasPermissionToLoad === 'true' || props.userId === props.creatorUserId,


I think it'd be clearer to do the comparison to the string at the point you take this out of localStorage.

lukebarnard1 · 2017-11-08T17:04:01Z

src/components/views/elements/AppTile.js

                appTileBody = (
                    <div className='mx_AppTileBody mx_AppLoading'>
                        <MessageSpinner msg='Loading...' />
                    </div>
                );
            } else if (this.state.hasPermissionToLoad == true) {
+                const loadingElement = (
+                    <div className="mx_AppTileBody">
+                        <AppWarning


could probably fit this on one line?

lukebarnard1 · 2017-11-08T17:05:12Z

src/components/views/elements/AppTile.js

                } else {
                    appTileBody = (
-                        <div className="mx_AppTileBody">
+                        <div className={this.loading ? 'mx_AppTileBody mx_AppLoading' : 'mx_AppTileBody'}>


I think you mean this.state.loading

lukebarnard1 · 2017-11-08T17:05:38Z

src/components/views/elements/AppTile.js

                } else {
                    appTileBody = (
-                        <div className="mx_AppTileBody">
+                        <div className={this.loading ? 'mx_AppTileBody mx_AppLoading' : 'mx_AppTileBody'}>
+                            { this.loading && Element }


Not sure what Element is

Yeah, that's just weird ... not sure how that got committed. Looks like autocomplete cruft? Embarrassing. Thanks for catching that.

Add error logging.

rxl881 · 2017-11-09T14:38:13Z

@lukebarnard1 - Can you please take another look? Thanks.

rxl881 · 2017-11-09T16:08:12Z

Depends upon - element-hq/element-web#5555

lukebarnard1

Could you make sure your comments are a reasonable width - some go up to 150 characters.

lukebarnard1 · 2017-11-10T11:00:57Z

package.json

@@ -74,6 +74,7 @@
    "matrix-js-sdk": "0.8.5",
    "optimist": "^0.6.1",
    "prop-types": "^15.5.8",
+    "qs": "^6.5.1",


I forgot about this. We already use querystring in riot-web. Let's use that? (See url_utils.js in riot-web)

Ok, I've updated again to reflect (I had querystring earlier in the PR, but reverted to qs as it is already being installed as a dep of other imports in react-sdk).

It's frustrating that riot-web's package.json doesn't list querystring as a dep., which would have made our use / choice of it more obvious.

lukebarnard1 · 2017-11-10T11:01:11Z

src/components/views/elements/AppTile.js

+     * Component initialisation is only complete when this function has resolved
+     */
+    setScalarToken() {
+        this.setState({initialising: true});


Could do this immediately preceding the getScalarToken? Then you don't have to do the initialising: false on line 138.

You do still need to always set initialising to false, as it is always set to true in the initialiser. Leaving this as is.

Oh, that's true.

rxl881 · 2017-11-10T11:52:18Z

I've reduced comment length.

If this is required, maybe this should be set in eslint - it's currently set to ignore?

lukebarnard1 · 2017-11-10T11:55:25Z

If this is required, maybe this should be set in eslint - it's currently set to ignore?

probably

rxl881 added 3 commits October 27, 2017 13:47

Unfinished, non-working changes to try and handle URL changes gracefully

7662b5a

Add URL search paramas polyfill

0a7273b

Check URL prop updates and ensure that widgets are refreshed.

3756ce6

rxl881 requested a review from lukebarnard1 October 27, 2017 16:53

rxl881 assigned lukebarnard1 Oct 27, 2017

rxl881 mentioned this pull request Oct 27, 2017

Use app url as key to ensure that widget is re-rendered on edit. #1542

Closed

lukebarnard1 suggested changes Oct 30, 2017

View reviewed changes

lukebarnard1 assigned rxl881 and unassigned lukebarnard1 Oct 30, 2017

rxl881 added 14 commits October 31, 2017 10:04

Removed comment

648b295

Formatting

1cb878b

Add querystring dep. already used by things like url_utils.js.

355d69b

Use querystring lib instead of URLSearchParams

6012b35

Use querystring lib

35b3326

Fix onLoad on wrong element.

758df29

Restructure to pass props from componentWillRecieveProps.

17c0405

Pass URL to check.

a52bb9d

Fix loading and initialisation spinners.

0e854ee

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

853ada0

into rxl881/widgetrendering

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

70c4100

into rxl881/widgetrendering

Fix url params parsing.

eb8c150

Fix URL parameter encoding.

d3784b4

Switch to using existing dep "qs" and record in package.json

96de72a

rxl881 assigned lukebarnard1 and unassigned rxl881 Nov 7, 2017

lukebarnard1 suggested changes Nov 8, 2017

View reviewed changes

rxl881 added 3 commits November 8, 2017 17:39

Remove unused dep.

ca1ffdf

Update variable name and JSdoc for improved clarity.

be0a76d

Formatting

b2b07d9

rxl881 added 4 commits November 8, 2017 20:38

Fix various loadingElement related issues.

56581ef

Fix broken commit.

8016fb8

Fix addition of scalar token to widget URL.

f796bc7

Add error logging.

Ensure that loading state is reset when showing app panel.

da8b1ff

rxl881 mentioned this pull request Nov 9, 2017

Don't display widget iframes whilst loading. element-hq/element-web#5555

Merged

rxl881 added 2 commits November 10, 2017 09:44

Explicitly set initialisation state.

98ac3dd

Improve function name.

bd6b5c4

lukebarnard1 suggested changes Nov 10, 2017

View reviewed changes

rxl881 added 2 commits November 10, 2017 11:42

Replace 'qs' dep. with 'querystring'

d2070a0

Comment length

ba8a9f2

lukebarnard1 approved these changes Nov 10, 2017

View reviewed changes

lukebarnard1 merged commit 6e1cf6c into develop Nov 10, 2017

rxl881 deleted the rxl881/widgetrendering branch November 13, 2017 18:18

Improve widget rendering on prop updates #1548

Improve widget rendering on prop updates #1548

Conversation

rxl881 commented Oct 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxl881 commented Nov 7, 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

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 Nov 9, 2017

rxl881 commented Nov 9, 2017

lukebarnard1 left a 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 Nov 10, 2017

lukebarnard1 commented Nov 10, 2017