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

Display warning if widget is mixed content #1263

Merged
merged 7 commits into from
Aug 2, 2017
Merged

Display warning if widget is mixed content #1263

merged 7 commits into from
Aug 2, 2017

Conversation

rxl881
Copy link
Contributor

@rxl881 rxl881 commented Aug 1, 2017

No description provided.

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@rxl881 rxl881 requested a review from kegsay August 1, 2017 16:31
errorMsg: PropTypes.string,
};
AppWarning.defaultProps = {
errorMsg: _t('Error'),
Copy link
Member

@kegsay kegsay Aug 1, 2017

Choose a reason for hiding this comment

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

You can't do _t here. It has to be in the render method (or function AppWarning in this case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, fair enough ... I should learn some more about the translation stuff.

@@ -207,6 +219,14 @@ export default React.createClass({
></iframe>
</div>
);
} else if (this.isMixedContent() == true) {
Copy link
Member

Choose a reason for hiding this comment

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

You can drop the == true and also does this work if hasPermissionToLoad=true but it's mixed content? 0:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, yes,... fixed.

const parentContentProtocol = window.location.protocol;
const u = url.parse(this.props.url);
const childContentProtocol = u.protocol;
if (parentContentProtocol === 'https:' && childContentProtocol !== parentContentProtocol) {
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 clearer to say:

if (parentContentProtocol === 'https:' && childContentProtocol !== 'https:') {
  // ...
}

IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@rxl881
Copy link
Contributor Author

rxl881 commented Aug 1, 2017

@kegsay - PTAL

@kegsay
Copy link
Member

kegsay commented Aug 1, 2017

LGTM

@kegsay
Copy link
Member

kegsay commented Aug 1, 2017

Need to wait for travis tests though.

@kegsay
Copy link
Member

kegsay commented Aug 1, 2017

/home/travis/build/matrix-org/matrix-react-sdk/src/components/views/elements/AppTile.js
  79:1  warning  Line 79 exceeds the maximum line length of 120  max-len
/home/travis/build/matrix-org/matrix-react-sdk/src/components/views/elements/AppWarning.js
  1:8  error  'React' is defined but never used  no-unused-vars

@kegsay
Copy link
Member

kegsay commented Aug 2, 2017

Tests still failing.

t3chguy
t3chguy previously requested changes Aug 2, 2017
const u = url.parse(this.props.url);
const childContentProtocol = u.protocol;
if (parentContentProtocol === 'https:' && childContentProtocol !== 'https:') {
console.warn("Refusing to load mixed-content app:", parentContentProtocol, childContentProtocol, window.location, this.props.url);
Copy link
Member

Choose a reason for hiding this comment

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

this line needs splitting across multiple for tests to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting that @t3chguy, sorted now :)

@kegsay
Copy link
Member

kegsay commented Aug 2, 2017

LGTM

@kegsay kegsay dismissed t3chguy’s stale review August 2, 2017 16:20

Addressed.

@rxl881 rxl881 merged commit 5752345 into develop Aug 2, 2017
@rxl881 rxl881 deleted the rxl881/warnings branch August 2, 2017 16:30
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.

4 participants