Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow only images in modal image uploader. #7672

Conversation

Ruk33
Copy link
Collaborator

@Ruk33 Ruk33 commented Aug 11, 2022

Fixes

Issue Number: #7646

What is the current behavior?

Even though a file filter is being used, the "All Files"
option allows users to select any file when uploading
an image.

What is the new behavior?

The electron filter is being used so the "All Files"
option is no longer displayed.

image

Other information

PR Checklist

Toggle...

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes)
  • Documentation changes
  • Other - Please describe:

Please check all that apply to this PR using "x":

  • I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • I added a line describing my change to CHANGELOG.md
  • I have checked that this PR does not introduce a breaking change
  • This PR introduces breaking changes and I have provided a detailed explanation below

return;
}
const file = new File([result.buffer], result.name);
this.props.onFileChosen(file);
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned that changing this from { path } to file will break publishing. Did you try?

const publishFormParams: { filePath: string | WebFile, name?: string, optimize?: boolean } = {
expects file.path on electron because the sdk needs the path.

It may be good to send { file: file , path: path } and adjust all the functions assigned to onFileChosen to take that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good @jessopb, nice catch. Perhaps as a small note for the future, we should avoid using these union types. Notice how I'm still respecting the interface (by sending a File type) but still, it manages to break parts of the app...

Copy link
Member

Choose a reason for hiding this comment

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

You're probably right. The union types were originally added when the codebase was supporting both web and app.

Maybe the type should be ChosenFile = { file: WebFile, path: string }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jessopb I have updated the code so the File now contains a path and a mime type. With this, the file upload for posts works fine. Please let me know what you think. Thanks.

this.props.onFileChosen({ path });
}
});
Object.defineProperty(file, 'path', {
Copy link
Member

Choose a reason for hiding this comment

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

This was an elegant solution, but I think WebFile is a well defined builtin type and probably shouldn't be polluted with extra params. I think we'd prefer { file: FILE, path: path }

Copy link
Member

@jessopb jessopb left a comment

Choose a reason for hiding this comment

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

Sorry it took a while to get to this. Tested publish and it's broken.

@@ -19,7 +19,7 @@ type Props = {
mode: ?string,
name: ?string,
title: ?string,
filePath: string | WebFile,
filePath: ?File,
Copy link
Member

Choose a reason for hiding this comment

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

Did you try publishing? Publish is trying to send an empty {}. (What happens when { thing: undefined })

@@ -11,10 +11,10 @@ import Icon from 'component/common/icon';

type Props = {
modal: { id: string, modalProps: {} },
filePath: string | WebFile,
filePath: File,
Copy link
Member

Choose a reason for hiding this comment

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

Also breaks here on file drop. This should be a path.

Copy link
Member

@jessopb jessopb left a comment

Choose a reason for hiding this comment

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

One small snag.

@@ -14,10 +14,13 @@ type Props = {

function ModalImageUpload(props: Props) {
const { closeModal, currentValue, title, assetName, helpText, onUpdate } = props;
const filters = React.useMemo(() => [{ name: 'Images', extensions: ['jpg', 'png', 'gif'] }]);
Copy link
Member

Choose a reason for hiding this comment

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

this blocks jpeg - possibly some other valid types. svg has been possible and allowed so far.

@jessopb jessopb merged commit 329d434 into master Sep 2, 2022
@jessopb jessopb deleted the 7646-in-the-cover-upload-modal-trying-to-upload-anything-other-than-an-image-doesnt-reset-the-form branch September 2, 2022 16:43
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.

None yet

2 participants