Skip to content

Conversation

@saul-prepared
Copy link
Contributor

What kind of change does this PR introduce?

  • Bug Fix
  • Feature
  • Refactoring
  • Style
  • Build
  • Chore
  • Documentation
  • CI

Did you add tests for your changes?

  • Yes, my code is well tested
  • Not relevant

If relevant, did you update the documentation?

  • Yes, I've updated the documentation
  • Not relevant

Summary
Explain the motivation for making this change. What existing problem does the pull request solve?
I created this Enhancement for this change.

Does this PR introduce a breaking change?
No

Other information

return fromDirEntry(entry) as any;
}

return fromDataTransferItem(item);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rolandjitsu I changed this to still call fromDataTransferItem while still getting the functionality I need.

…dd functionality to fromDataTransferItem instead.
@saul-prepared
Copy link
Contributor Author

@rolandjitsu Could you please review this again. I made the necessary changes.

Copy link
Collaborator

@rolandjitsu rolandjitsu left a comment

Choose a reason for hiding this comment

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

@saul-prepared thanks for your contribution and sorry for the delayed reply. There are a few more changes needed that I hope make sense to you.

Comment on lines +108 to +120
Object.defineProperty(f, 'relativePath', {
value: typeof path === 'string'
? path
// If <input webkitdirectory> is set,
// the File will have a {webkitRelativePath} property
// https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/webkitdirectory
: typeof webkitRelativePath === 'string' && webkitRelativePath.length > 0
? webkitRelativePath
: `/${file.name}`, // prepend forward slash (/) to ensure consistancy when path isn't supplied.
writable: false,
configurable: false,
enumerable: true
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not too different from setting the path. Let's make this block a function and reuse it for setting the path and relativePath.

Also, since the path is relative, /${file.name} should probably be ./${file.name}.

], []);
}

function fromDataTransferItem(item: DataTransferItem) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function fromDataTransferItem(item: DataTransferItem) {
function fromDataTransferItem(item: DataTransferItem, entry?: FileSystemEntry) {

return Promise.reject(`${item} is not a File`);
}
const fwp = toFileWithPath(file);
const fwp = toFileWithPath(file, fileAsEntry?.fullPath ?? undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const fwp = toFileWithPath(file, fileAsEntry?.fullPath ?? undefined);
const fwp = toFileWithPath(file, entry?.fullPath ?? undefined);

Comment on lines +125 to +130

let fileAsEntry: FileSystemEntry | null = null;
if (typeof item.webkitGetAsEntry === 'function') {
fileAsEntry = item.webkitGetAsEntry();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can get rid of this if on line 111 you add this:

else if (entry) {
  return fromDataTransferItem(item, entry);
}

Because you already have the entry in the toFilePromises func.

@rolandjitsu
Copy link
Collaborator

@saul-prepared if you'd like, I could take over the PR and make the necessary changes.

@rolandjitsu rolandjitsu mentioned this pull request Oct 27, 2024
12 tasks
@rolandjitsu
Copy link
Collaborator

I've taken over the changes in #99.

@rolandjitsu rolandjitsu closed this Nov 2, 2024
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.

2 participants