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

Saving a document doesn't show correct origin #8786

Merged
merged 1 commit into from
May 10, 2017

Conversation

kumarrishav
Copy link
Contributor

@kumarrishav kumarrishav commented May 10, 2017

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Fix #8698

Test Plan:
Open browser
Download file having orgin file:/// or chrome-extension: or other local origin.
It should show text 'Local file' for local origin file/
screen shot 2017-05-10 at 5 48 05 pm

@@ -73,6 +73,8 @@ class DownloadItem extends ImmutableComponent {
}
render () {
const origin = getOrigin(this.props.download.get('url'))
const localFileOrigins = ['file:', 'blob:', 'data:', 'chrome-extension:', 'chrome:']
const localFile = localFileOrigins.some((localFileOrigin) => origin.startsWith(localFileOrigin))
Copy link
Member

Choose a reason for hiding this comment

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

this should check that origin is not null

Copy link
Contributor Author

@kumarrishav kumarrishav May 10, 2017

Choose a reason for hiding this comment

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

Did you mean like this: origin && localFileOrigins.some((localFileOrigin) => origin.startsWith(localFileOrigin)) @diracdeltas

@@ -160,7 +162,7 @@ class DownloadItem extends ImmutableComponent {
? <span className='fa fa-unlock isInsecure' />
: null
}
<span title={origin}>{origin}</span>
<span title={origin}>{localFile ? 'Local file' : origin}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add translation here

})
})

Object.values(downloadStates).forEach(function (state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you could do this tests insider previous forEach

@kumarrishav
Copy link
Contributor Author

@diracdeltas @NejcZdovc review?

@kumarrishav kumarrishav self-assigned this May 10, 2017
@NejcZdovc NejcZdovc added this to the 0.15.400 milestone May 10, 2017
@bsclifton
Copy link
Member

@kumarrishav great job on this 😄

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

Successfully merging this pull request may close these issues.

5 participants