Skip to content

Comments

Add encodeUriComponent to file name#967

Merged
epicfaace merged 5 commits intorjsf-team:masterfrom
jorgerobles:patch-1
Jan 20, 2019
Merged

Add encodeUriComponent to file name#967
epicfaace merged 5 commits intorjsf-team:masterfrom
jorgerobles:patch-1

Conversation

@jorgerobles
Copy link
Contributor

Reasons for making this change

Fix for non standard file names (with spaces, apostrophes, etc) that prevent to be used.
For example uploading a whitespace named file prevents to be used as background url.

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Fix for non standard file names (with spaces, apostrophes, etc) that prevent to be used.
import { dataURItoBlob, shouldRender, setState } from "../../utils";

function addNameToDataURL(dataURL, name) {
name=encodeURIComponent(name)
Copy link
Contributor

@graingert graingert Jul 23, 2018

Choose a reason for hiding this comment

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

you've got a lint error here.

Do this instead:

  function addNameToDataURL(dataURL, name) {
    return dataURL.replace(";base64", `;name=${encodeURIComponent(name)};base64`); 
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @graingert, did not see your comment until now.

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

@jorgerobles Thanks for the PR! Do you still plan to work on this? In addition to the lint error, I would suggest you also add a test.

Copy link
Contributor Author

@jorgerobles jorgerobles left a comment

Choose a reason for hiding this comment

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

Forgot to fix it 🤦‍♂️

@epicfaace
Copy link
Member

@jorgerobles
Copy link
Contributor Author

@epicfaace I've added the test. I'm not very familiarised with mocha, hope is well.

@epicfaace
Copy link
Member

@jorgerobles Thanks, almost there -- instead of modifying the existing test, can you leave the existing test case "should reflect the change into the dom" in place and add a new test case right next to it? (you can copy and paste the existing test case and make the changes that you did)

describe("FileWidget", () => {
const initialValue = "data:text/plain;name=file1.txt;base64,dGVzdDE=";

const nonUriEncodedValue = "fileáéí óú1.txt";
Copy link
Member

Choose a reason for hiding this comment

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

Please move these two lines inside the "should encode file name with encodeURIComponent" test 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.

👍

@epicfaace epicfaace merged commit 75571ba into rjsf-team:master Jan 20, 2019
@epicfaace
Copy link
Member

Thank you, @jorgerobles !

@jorgerobles
Copy link
Contributor Author

jorgerobles commented Jan 20, 2019 via email

@epicfaace
Copy link
Member

@jorgerobles can you help with #824? It's pretty similar to this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants