Skip to content

Conversation

@ThisIsOstad
Copy link
Contributor

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 os-js/OS.js#752.

@andersevenrud andersevenrud self-assigned this Jan 9, 2021
@andersevenrud andersevenrud added the enhancement New feature or request label Jan 9, 2021
@andersevenrud andersevenrud self-requested a review January 9, 2021 19:44
@andersevenrud
Copy link
Member

I'll do a proper review of this in a little bit. But so far it looks really good! 👍

There's one piece of the puzzle here remaining, which is noted in os-js/OS.js#752 (comment).

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 🤔

@ThisIsOstad
Copy link
Contributor Author

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?

Copy link
Member

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

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

Can you amend the commit 981d0a4 message to say the following:

Fixed typo in clipboard type definitions

Copy link
Member

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

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

Can you amend the commit 1f1548b message to reference the following issue I've opened about this:

#144

@andersevenrud
Copy link
Member

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.

@andersevenrud andersevenrud linked an issue Jan 9, 2021 that may be closed by this pull request
2 tasks
@andersevenrud
Copy link
Member

Looks good!

Now, I'm a bit nit-picky 😂 Would it be too much of me to ask you to squash the following commits together ?

@andersevenrud
Copy link
Member

My man! I'll paste my gitter response about the loading strategy in #144 and we can take the rest from there.

@andersevenrud
Copy link
Member

Man, you're fast! Gotta say I'm impressed 🤓

@ThisIsOstad
Copy link
Contributor Author

It seems that something is broken 😅 Do you know what's the problem?

@andersevenrud andersevenrud self-requested a review January 9, 2021 23:00
Copy link
Member

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

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

The things I've commented in this request is about methods that could be separated from this class as they do not directly rely on it.

How do you feel about this ?

@andersevenrud andersevenrud changed the title Add middleware provider [WIP] Add middleware provider Jan 9, 2021
@andersevenrud
Copy link
Member

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.

@andersevenrud
Copy link
Member

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 😅 I really enjoyed this though! Very fast response, good insight and feedback, and you really seem to know your stuff. Love it! hehe

@ThisIsOstad
Copy link
Contributor Author

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 ✌️

@andersevenrud
Copy link
Member

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.

@ThisIsOstad
Copy link
Contributor Author

@andersevenrud I found a bug (the url was not correct) while testing it with filemanager application. It's fixed now.

@andersevenrud
Copy link
Member

Where in the code was this ? I can't see the actual fix because of the force push 😊

@andersevenrud
Copy link
Member

I think something went wrong with your amend because the commit message changed ?!

It's now Added support for middleware in edit menu

@ThisIsOstad
Copy link
Contributor Author

@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?

@andersevenrud
Copy link
Member

If you mean the conversation I started (#143 (comment)), then yeah. I answered and resolved that.

@andersevenrud andersevenrud changed the title [WIP] Add middleware provider Add middleware provider Jan 17, 2021
@andersevenrud
Copy link
Member

PR in FileManager: os-js/osjs-filemanager-application#31

@andersevenrud andersevenrud merged commit a7c4d39 into os-js:master Jan 17, 2021
@ThisIsOstad
Copy link
Contributor Author

I'm getting the following new warnings with these changes:

Screen Shot 1399-10-28 at 23

Do you have any idea what's the issue?

@andersevenrud
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add generic middleware support Adding context menu to file manager according to packages installed on OSjs.

2 participants