Skip to content

Conversation

rodrigomata
Copy link

Fixes (#44,#62)

Solution to @mickbalaban's PR

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.5%) to 89.873% when pulling 71d0b09 on rodrigomata:master into 332dfde on abdennour:master.

window.navigator.msSaveBlob(blob, filename)

return false
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

handleLegacy is responsible for handling cases where the anchor element's href attribute cannot be used to trigger the download. This else block is unnecessary as we could just move this logic to the buildURI function and have it return that straight to the href attribute on line 58. That would reduce the complexity of the changes from this PR which would subsequently make it easier for the code coverage to remain above 90%.

Let me know if you are able to make those changes, otherwise I am happy to implement them myself in a new PR (as I have been meaning to do)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Arguably the function should be renamed, since the function's purpose is to support IE11, which requires us to use msSaveBlob, as it doesn't support href="data:"

@athurman
Copy link

Hey! just wondering what the current status is for getting this merged in? thanks

@mriccid
Copy link
Collaborator

mriccid commented Feb 28, 2018

@athurman if the changes requested above are made and the test coverage is not negatively affected then I would be happy for this to be merged in and released.

@tienthanhdhcn
Copy link

Please could anyone let me know when the problem will get fixed??

@mriccid
Copy link
Collaborator

mriccid commented Mar 6, 2018

I've started on a PR to fix this problem because I don't think this PR is moving.
#73

I need to fix a few small issues with it and ensure that the build passes, then will get it merged

@tienthanhdhcn
Copy link

@mriccid thank you very much. I tried to use the fix from #44 and found that we need to move evt.preventDefault() outside the "if" statement in "handleLegacy" to prevent double downloading the file.

@mriccid mriccid force-pushed the master branch 2 times, most recently from 4c691cb to f129262 Compare February 5, 2019 05:11
@mccabemj
Copy link
Collaborator

mccabemj commented Feb 5, 2019

As far as I can see, this was dealt with in #73

@mccabemj mccabemj closed this Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants