search

ngyuki / vscode-ghosttext

GhostText support for Visual Studio Code
2 stars 1 forks source link

Hide notification? #4

Closed fregante closed 3 years ago

fregante commented 3 years ago

Hi! I found your fork and it seems to work better than the existing ones, but I noticed that there's a notification that can't be closed. Can this be avoided? The connection can be closed by closing the tab, this notification shouldn't be necessary.

Screen Shot 1
viking1304 commented 3 years ago

@fregante if you have some free time, check out my fork https://github.com/viking1304/vscode-ghosttext

I completely commented this notification few minutes ago, but I didn't had time to test it.

viking1304 commented 3 years ago

@ngyuki I tested my modifications and everything looks fine.

I would really aprichiate if you could merge those changes (or completely remove notification by yourself), since I don't like the idea that I have to publish slightly modified version of your work to marketplace, just because you don't have enough free time.

ngyuki commented 3 years ago

@fregante @viking1304 If you click cancel button or close tab, the notification will also close. This notification shows editing site name and page title. I think this is useful.

Or... Do you mean that click cancel button, or even close tab, does not close the notification?

viking1304 commented 3 years ago

@fregante @viking1304 If you click cancel button or close tab, the notification will also close. This notification shows editing site name and page title. I think this is useful.

Or... Do you mean that click cancel button, or even close tab, does not close the notification?

It is impossible to close the notification window, but to continue editing. Closing the notification also closes the connection (at least in latest VSC). Even your intentions were good, this might be misleading, even annoying. As a workaround I just completely commented out your code related to that notification window.

When I think about it now, I guess removing just the cleanup from line 67 should be enough. That should just remove the notification without closing the connection when user click on cancel. If you remove that part, progress would not be needed as well.

Also, StatusBarItem probably would be much better choice than notification window.

ngyuki commented 3 years ago

@viking1304 It is intended that closing the notification will also close the connection.

Even your intentions were good, this might be misleading, even annoying.

What kind of misleading can you think of?

viking1304 commented 3 years ago

@ngyuki I am a web developer and a part of our UX team, so I showed your extension and how it works to the rest of my team and asked them what they think about it. They all said that showing notification that can't be closed without closing the connection and interrupting users work is not really good UX practice.

Main purpose of any notification in any software is to inform user about something. User should have option to close that notification and to continue working, except if that is any kind of failure message. Most users will expect exactly that, that they will just close the notification, but they will close everything instead. That's why I wrote that it might be misleading.

Cancel should just close the notification, but you can add another button (or link) that will actually close everything if you want, but it must be clearly labeled as Close or something similar.

I also asked them what they think about status bar item, and they also agreed that in this case that might be better approach than floating notification.

All this is just a friendly advice without any bad intention, and as far as I understood @fregante (author of GhostText and a person who opened this issue) also agrees with me. @fregante please correct me if I am misinterpret something.

ngyuki commented 3 years ago

@viking1304 Thank you for your detailed comment!

As you pointed out, I don't think label "cancel" is appropriate. However, I could not find a way to change this label.

On the other hand, editing page title and site name, is quite long, so we think it is not appropriate to display it in the status bar.

it mimicked ctf0/ghosttext-vscode, this extension seems to keep the connection even after closing the notification window. So maybe I'm wrong about that.

I also found the bug in #6, so I will remove the cleanup when the user clicks cancel, as you commented.