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

Fixes #9667 #9860

Merged
merged 2 commits into from
Nov 2, 2018
Merged

Fixes #9667 #9860

merged 2 commits into from
Nov 2, 2018

Conversation

miphilomath
Copy link
Contributor

@miphilomath miphilomath commented Oct 31, 2018

Changed the file type separator with comma(,) and added a function to verify the file upload type.

Signed-off-by: miphilomath 26297035+miphilomath@users.noreply.github.com

  • This PR relates to an existing open issue and there are no existing
    PRs open for the same issue.
  • Add Fixes #ISSUENUM at the top of your PR.
  • Add a description of the the changes introduced in this PR.

… verify the file upload type.

Signed-off-by: miphilomath <26297035+miphilomath@users.noreply.github.com>
@miphilomath
Copy link
Contributor Author

@eviljeff Please review :)

mozilla/addons#6076

return file ? file : preLoadBlob;

Copy link
Member

Choose a reason for hiding this comment

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

unnecessary extra line.

@@ -11,7 +11,10 @@ $(document).ready(function() {
function getFile() {
file_selector = $wizard.find('#header-img')[0];
file = file_selector.files[0];
if (!file.name.match(/.(jpg | jpeg | png | gif | apng)$/i))
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Also, you've got some strange indentation :)

Copy link
Contributor Author

@miphilomath miphilomath Nov 1, 2018

Choose a reason for hiding this comment

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

Ah! Must be my editor. Let me reformat these lines.

Update: My editor used Tabs that is why the odd indentation.

@@ -11,7 +11,10 @@ $(document).ready(function() {
function getFile() {
file_selector = $wizard.find('#header-img')[0];
file = file_selector.files[0];
if (!file.name.match(/.(jpg | jpeg | png | gif | apng)$/i))
Copy link
Member

Choose a reason for hiding this comment

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

This works great (apart from missing .svg off) but it'd be good to re-use the accept attribute from the html so the list of acceptable files is only defined in one place.
How about:
if (file && $wizard.find('#header-img').attr('accept').split(',').indexOf(file.type)== -1) ?
It uses file.type to get content type which should hopefully be more robust too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow! That looks like a more clean and efficient way of doing the task.
Thanks :)

Signed-off-by: miphilomath <26297035+miphilomath@users.noreply.github.com>
@eviljeff
Copy link
Member

eviljeff commented Nov 2, 2018

r+ thanks for the contribution

@eviljeff eviljeff merged commit b97ef5b into mozilla:master Nov 2, 2018
@caitmuenster
Copy link

Thanks, @miphilomath, and welcome onboard! 🎉Your contribution has been added to our recognition wiki.

I look forward to seeing you around the project!

@miphilomath
Copy link
Contributor Author

Thanks @caitmuenster @eviljeff! I wanted to meet you guys at MozFest but couldn't due to my volunteering duties, May be next time :)

@miphilomath miphilomath deleted the 9667-Prevent-unsupported-image-types branch March 25, 2019 19:13
MelissaAutumn pushed a commit to thunderbird/addons-server that referenced this pull request Aug 25, 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.

3 participants