estruyf / vscode-squarl

Squarl Bookmarks - Bookmark important files and/or links for your projects.
https://marketplace.visualstudio.com/items?itemName=eliostruyf.vscode-squarl
MIT License
4 stars 1 forks source link

new feature: Added promote and demote commands #3

Closed Adam-it closed 2 years ago

Adam-it commented 2 years ago

🎯Aim

The aim this PR is to functionality to promote local link to global one and demote (the otherway around)

βœ…ToDo

πŸ“· Result

https://user-images.githubusercontent.com/58668583/192648853-32256095-1134-4403-9605-188a0ab05f54.mp4

πŸ€”Remarks

  1. Not sure should I try to unit test this code ?

πŸ”— Related Issue

Closes: #2

Adam-it commented 2 years ago

@estruyf I am not sure how to approach showing/hidding promote/demote for local/global links πŸ˜‰. initially I was thinking to add a new context key to a custom method check to check if selected item has isGlobal === true but I wasn't sure if maybe this was a bit of an overkill πŸ€”. Maybe you know some fancy condition I could use in the when property? I checked the docs but didn't notice if it is possible to reference the actual item and it's property πŸ€” (not much experience here πŸ˜‰)

Besides that I am also not sure of the icons I added to the commands but I guess they do not show anyway right ? πŸ˜‰

estruyf commented 2 years ago

Great job @Adam-it!

There no need to add a new context value. What we can do, is highlight that a bookmark is global. For instance, instead of having the value bookmark as a context value, we can add bookmark.global. That is something we can add in the CreateBookmark.ts file:

contextValue = contextValue ? `${contextValue}Bookmark` : `bookmark`;
if (bookmark.isGlobal) {
  contextValue = `${contextValue}.global`;
}

In the when clauses, we can then do the implementation as follows:

// Old line
"when": "view == squarl.view.project && viewItem =~ /bookmark|group/",

// New line
"when": "view == squarl.view.project && viewItem =~ /^bookmark|group/",

The exact matches need to be changed to the starts with matches as well:

// Old line
"when": "view == squarl.view.project && viewItem == bookmark",

// New line
"when": "view == squarl.view.project && viewItem =~ /^bookmark/",

With these changes in place, you accomplish what you're looking for.

Adam-it commented 2 years ago

Great job @Adam-it!

There no need to add a new context value. What we can do, is highlight that a bookmark is global. For instance, instead of having the value bookmark as a context value, we can add bookmark.global. That is something we can add in the CreateBookmark.ts file:

contextValue = contextValue ? `${contextValue}Bookmark` : `bookmark`;
if (bookmark.isGlobal) {
  contextValue = `${contextValue}.global`;
}

In the when clauses, we can then do the implementation as follows:

// Old line
"when": "view == squarl.view.project && viewItem =~ /bookmark|group/",

// New line
"when": "view == squarl.view.project && viewItem =~ /^bookmark|group/",

The exact matches need to be changed to the starts with matches as well:

// Old line
"when": "view == squarl.view.project && viewItem == bookmark",

// New line
"when": "view == squarl.view.project && viewItem =~ /^bookmark/",

With these changes in place, you accomplish what you're looking for.

This is awesome 🀩🀩. Thanks for the pro tip πŸ™‚. Learning a lot here πŸ’ͺ

I will try to implement those changes in the coming days πŸ‘

Adam-it commented 2 years ago

@estruyf thanks for the tip. Done in that way. Also while testing notice one small bug that the user could lose the link if she or he declined when picking group (as first I was removing the old bookmark, then asking for group and then adding new one). I reordered this behavior.

Ready for review. Let me know if I didn't miss anything πŸ™‚

estruyf commented 2 years ago

Thanks @Adam-it - going to give it a try.

estruyf commented 2 years ago

Thanks @Adam-it, I have tested it and it works! Just added a retry logic to the setSettings method, as one time it did not promote the bookmark (removed from workspace, only group got created, the link was lost). After the retry it never happened anymore.

Adam-it commented 2 years ago

Ok I saw your change. Thanks for that πŸ‘. Thanks again for the opportunity, for sure I learned a lot, especially how to keep a clean solution 😯 and how to do a really cool activitybar view but not using a webview (which I usually did.... Ok I still do 😝). I plan to reuse some of your approach in the next extension I will do which is suppose to help out new contributors to the CLI for Microsoft 365. If you will have any more ideas for this extension maybe open an issue and let me know πŸ‘. Would love to help out again. Especially since I will be using this one for sure 😍.

estruyf commented 2 years ago

Using a webview is tempting, as it gives more control over the things you can do. Maybe this extension will eventually use a webview as well, depending on the features we'll add in the future.

Thanks again for your contribution! I will release the first official version today now that all the base features are in place.

If you have more ideas/feedback, you are more than welcome to share these or work on them.