Skip to content
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

Incorporate Vanilla-Notify for HUD on Entering Insert/Normal Mode #181

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

eizKjfRsZaGPkcNCbcAZ
Copy link

Incorporated MLaritz/Vanilla-Notify with slight modifications.

All credit goes to @MLaritz, the author of Vanilla-Notify.

A translucent sticky info box appears at the bottom right upon entering the Insert Mode. The box is replaced by a non-sticky notification upon exiting back to the Normal Mode.

screenshot

Things to improve: 1. Cleaning out the !important property in injected.css. 2. Adding GUI options for selecting info box / notification styles.

@nbelzer
Copy link
Collaborator

nbelzer commented Aug 11, 2020

@agbohub Thanks for this, as mentioned in #176 we should be more transparent to the user when insert mode is entered. I tested the branch locally but was not greeted with a popup as shown in your provided screenshot on entering insert mode.

A few points I would like to see before merging this:

  • Make sure the popup appears (could be a problem on my side, however I wasn't able to find the logic for making the popup appear in the pushed code).
  • Move the library code for Vanilla Notify out of injected.js into it's own file, same as is done for the mousetrap library.
  • The same applies to the required css.
  • There is a similar type of popup system that is already used when for example pressing f. If you have the time it might be interesting to see if you can replace that system with yours so that all popups are similar.

Screenshot 2020-08-11 at 13 39 17

If you need help, I'd be glad to help out either here on Github or through chat on twitter (@nickbelzer).

eizKjfRsZaGPkcNCbcAZ and others added 5 commits August 12, 2020 08:58
- Move vnotify scripts into ./Vimari Extension/js/lib/.
- Add option to enter Insert Mode automatically for insertModeUrls in config settings. Useful for cases such as youtube.com/watch
@eizKjfRsZaGPkcNCbcAZ
Copy link
Author

@nbelzer Thank you for the great feedback. I just pulled a new request that addresses the following issues.

  • Make sure the popup appears (could be a problem on my side, however I wasn't able to find the logic for making the popup appear in the pushed code).

Could you please try the new commit? I tested it on a new macOS install and it worked fine.

  • Move the library code for Vanilla Notify out of injected.js into it's own file, same as is done for the mousetrap library.

Done. The library code for Vanilla Notify is now at ./Vimari Extension/js/lib/.

I am still working on separating css files and potentially integrating with the existing popup system.

I also added a new option which makes Vimari automatically enter Insert Mode for user-defined URLs in config settings under insertModeUrls. This is helpful for the likes of youtube.com/watch where you want to default into using the built-in playback control shortcuts.

@nbelzer
Copy link
Collaborator

nbelzer commented Aug 12, 2020

Thanks for working on this.

Could you please try the new commit? I tested it on a new macOS install and it worked fine.

The popup now appears for me as well.

I am still working on separating css files and potentially integrating with the existing popup system.

Nice, I would suggest moving the css code into a separate file, similarly to what you did with the popup library.

I also added a new option which makes Vimari automatically enter Insert Mode for user-defined URLs in config settings under insertModeUrls. This is helpful for the likes of youtube.com/watch where you want to default into using the built-in playback control shortcuts.

This is great, however I have to ask you to move these changes to a different branch which would allow you to create a separate pull request for this. This is really a separate feature from the popup functionality, by including it in this PR you are making it hard to review the code changes and discuss the different functionalities.

@eizKjfRsZaGPkcNCbcAZ eizKjfRsZaGPkcNCbcAZ changed the title Incorporate Vanilla-Notify Incorporate Vanilla-Notify for HUD on Entering Insert/Normal Mode Aug 12, 2020
@eizKjfRsZaGPkcNCbcAZ
Copy link
Author

eizKjfRsZaGPkcNCbcAZ commented Aug 12, 2020

Thank you @nbelzer.

I have separated the vnotify.css file and reverted changes related to insertModeUrls (I will create a new PR for this feature later).

Also, per issue #184, I commented out the HUD-related lines in injected.js and replaced them with vNotify.

@eizKjfRsZaGPkcNCbcAZ
Copy link
Author

Updated files changed to conform with v2.1.0-beta2.

@nbelzer Please don't hesitate to let me know if there is anything else that needs to be done before this pull gets merged. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants