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

Editor: Make sure to log an unsupported extension #19592

Merged
merged 2 commits into from
Jun 8, 2020

Conversation

gero3
Copy link
Contributor

@gero3 gero3 commented Jun 8, 2020

I wanted to load an exr file in the editor. So I tried to import the file in the editor. Nothing seemed to happen. Checking the dev tools didn't help either. The exr file isn't supported in the editor, I found that out by debugging the editor.

To Reproduce:

  • Import an exr file in the editor

Issue

  • Nothing happens

Solution

  • Log if the extension is not supported and make sure not to block because of multiple files loading.

@Mugen87 Mugen87 changed the title Make sure to log an unsupported extension Editor: Make sure to log an unsupported extension Jun 8, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 8, 2020

make sure not to block because of multiple files loading.

That's the reason why not using alert(), right? I still think that using alert() is better in this context since a lot of users (beginners, non-developers) don't check the browser console.

@gero3
Copy link
Contributor Author

gero3 commented Jun 8, 2020

That's the reason why not using alert(), right? I still think that using alert() is better in this context since a lot of users (beginners, non-developers) don't check the browser console.

I didn't use alerts because @mrdoob commented that line specifically when adding multiple file support. But I'm willing to change it to an alert because it is worse without warning the users.

@Mugen87 I'll update it if you still think so.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 8, 2020

Um, I see. In this case I vote to only change the default branch in this PR and don't change the existing usage of alert().

@mrdoob mrdoob added this to the r118 milestone Jun 8, 2020
@mrdoob mrdoob merged commit aea23e6 into mrdoob:dev Jun 8, 2020
@mrdoob
Copy link
Owner

mrdoob commented Jun 8, 2020

Thanks!

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

3 participants