Closed ThisIsOstad closed 3 years ago
I'll do a proper review of this in a little bit. But so far it looks really good! :+1:
There's one piece of the puzzle here remaining, which is noted in https://github.com/os-js/OS.js/issues/752#issuecomment-703812143.
Since packages needs to register this middleware before the application is launched, there needs to be some kind of action that loads a script from the package without actually launching the package (application).
This could probably be done in another PR though :thinking:
Thanks 😊
Yes, I will create another PR (soon 😁) in the file manager application to make use of this middleware. And then, I think we can go for adding the middleware.js and its preload strategy in any application that needs to utilize a middleware. Is it right or we need to handle the preload strategy in this PR?
Is it right or we need to handle the preload strategy in this PR?
It's OK to commit to this PR if you want.
Looks good!
Now, I'm a bit nit-picky :joy: Would it be too much of me to ask you to squash the following commits together ?
My man! I'll paste my gitter response about the loading strategy in #144 and we can take the rest from there.
Man, you're fast! Gotta say I'm impressed :nerd_face:
It seems that something is broken 😅 Do you know what's the problem?
Codeclimate brings up a good point in this latest report. These blocks could be replaced with a method in the Packages
class that accepts the "file type" and "package type" as parameters.
Good stuff!
Sorry if I dragged this process out a bit, but I'm a bit of a perfectionist when it comes to certain things :sweat_smile: I really enjoyed this though! Very fast response, good insight and feedback, and you really seem to know your stuff. Love it! hehe
I think it was easier and faster for you to do it yourself, so thanks for letting me working on it. I really enjoyed working on this issue with you ✌️
I'm going to test this out myself tomorrow (well... later today, it's 2AM and going to sleep soon) to make sure this is ready for a merge.
@andersevenrud I found a bug (the url was not correct) while testing it with filemanager application. It's fixed now.
Where in the code was this ? I can't see the actual fix because of the force push :blush:
I think something went wrong with your amend because the commit message changed ?!
It's now Added support for middleware in edit menu
@andersevenrud Sorry for the commit message it was because I was working on filemanager at the same time.
I added a message on the code I've changed, did you see that?
If you mean the conversation I started (https://github.com/os-js/osjs-client/pull/143#issuecomment-761857366), then yeah. I answered and resolved that.
PR in FileManager: https://github.com/os-js/osjs-filemanager-application/pull/31
I'm getting the following new warnings with these changes:
Do you have any idea what's the issue?
These changes does not affect anything related to that, and those messages are normal :)
They go away when you activate the desktop icons and put some stuff in there.
It adds
osjs/middleware
provider to the core. As an example, it can be used to dynamically add items to the context menu in the file manager application.Closes https://github.com/os-js/OS.js/issues/752.