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: Exposed supported formats in Loader.js; #28324
Conversation
Hmm, doesn't it make the code less readable? IMHO maintaining the |
see #28295 (comment) |
@Mugen87 what do you think? |
In short, this PR can be closed if ... hardcoded supported file formats is accepted for bith file input `accept` value, and the supported file formats hint in UI feedback :D, Expand it to see why refactoring is needed, thanks.Why refactor: Editor supports two ways of importing files from fs: |
29c24e3
to
c5f0406
Compare
Fair enough! Since there is now more than one use case that uses the file format list I'm okay with adding it. |
Do you mind resolving the merge conflicts as well? |
done |
As far as I understand, the main purpose of this PR was to enable Now that #28396 has made the method no longer needed, should we revert this PR? |
TBH, there is no reason anymore for the refactoring. If you prefer the previous code style, I'm fine with reverting, too. |
What code style do you prefer? |
The previous one, tbh. If I would implement the module from scratch, I would rather use a switch/case statement like before than storing the handlers in an object. |
We appreciate all the improvements you're doing to the editor. However, it may be better to share what you're planing to work on before spending a lot of time working on a PR that changes a lot of files or refactors code. If you do that, we may be able to help you on coming up with simpler solutions and save you time. |
Thanks.
Had been in #28324 (comment) I should write it at the OP sorry for that 🙏🏻
This PR was not originally for #28396, instead it was for #28295, (I even added 'TODO: UI Feedback' in commit :D), so no. |
Why refactor:
Editor supports two ways of importing files from fs: accept does guard users from selecting unsupported files in the first place for File>Import, however there's no equivalent facility for the 2nd way, drag n drop, so DIY UI feedback is needed for notifying users that no-op as they dropped unsupported files, I prefer showing Import Error: Unsupported file format (xxx) together with editor's supported file formats (same purpose of accept that supported file formats are listed in browser file dialog), my attempt was using #28295 and #28295 (comment), so 'introspection of supported formats' is inevitable, that's why file handlers are re-organized to help introspect in getSupportedFileFormats().
The PR:
Extracted file handlers into
fileHandlers
object:(editor, manager, reader, file, ...handlerSpecificOptions): void
fileHandlers[ 'myFormat' ] = myFormatHandler
fileHandlers
Exposed
getSupportedFileFormats()
:fileHandlers
objectsort()
getSupportedFileFormats()
to generateaccepts
list for File>Import input (not included in this PR)