GaurangTandon
commented
6 years ago Hey, could I please get a response on this?
Open GaurangTandon opened 6 years ago
GaurangTandon
commented
6 years ago Hey, could I please get a response on this?
makyen
commented
6 years ago It looks helpful/useful.
For myself, I don't use the keyboard shortcuts. Thus, while I can test it on SO, MSO, and MSE, I wouldn't be actively using it, which might find additional issues. Personally, I tend to use a one hand on keyboard and one hand on mouse style. I have a userscript which shows a single-click close-vote action on all questions (currently, SO only; works on question pages, reviews, and MagicTag, but needs to be updated to function on NATO with NATO Enhancements).
I did, relatively briefly, look at the code and can give you some comments on that if you want.
Some quick comments (not a complete review):
hasClass is effectively Element.classList.contains(). It's better to use the already existing standard method.$() unless you're actually using jQuery (or other standard lib). Using $() is confusing. People will assume that the code is using one of the typical libraries. If you want to roll your own helper functions, or a complete library, then pick something other than $ as the name for your primary interface. Personally, I chose Q() and q() when I did it.keyup, keypress might be a more appropriate event to listen to & should be the one which will need to be handled when resolving the Firefox issues. If you do use it, the normal use of those keys should be prevented during the times your script is actively looking for that input (e.g. backspace is only used by your script when a popup is open).if, for, etc. that spans multiple lines, please use code blocks, i.e. {}. Not using them is basically just asking for issues when someone else maintains the code. (Yes, I know that's not necessarily consistent with code already in this repository, but that's a larger issue, which is on my list to discuss with the other stakeholders here).var style, then put each definition on a new line. When they are on the same line, the definitions can often be easy to overlook.#content. While it's probably not too bad, there are much more efficient methods of finding out that a popup has been opened. If you do use one, please only call your popup-search at most once every 100ms, or so, not on every time the MutationObserver fires, which can be thousands of times a second.getVisiblePopup you're doing a complete DOM-walk for each of the POPUP_IDS. You could easily do it in one DOM-walk, while getting all of them with $('#' + Object.values(POPUP_IDS).join(', #')).HTMLElement.click() method didn't work on some elements? Is that the reason you're using your invokeClick() function and calling .dispatchEvent(). Not that doing so is wrong, it's just a bit verbose.GaurangTandon
commented
6 years ago Hi again. Thanks for your excellently detailed feedback. I have incorporated all of it in the latest release: PopupDialogShortcutsSE.user.js
Please do have a look.
The only issue that couldn't be worked out was "keypress...should be the one which will need to be handled when resolving the Firefox issues." It did fix the backspace issue, but it didn't fix the "Search for text when typing" issue.
If you do use one, please only call your popup-search at most once every 100ms
I have throttled the MutationObserver handler to 250ms, with no visible effect on the results.
In getVisiblePopup you're doing a complete DOM-walk for each of the POPUP_IDS. You could easily do it in one DOM-walk, while getting all of them with $('#' + Object.values(POPUP_IDS).join(', #')).
I ended up removing the hardcoded IDs entirely, since every popup (and only a popup) has the unique selector div.popup[id*='popup']
Just wondering: In you development, did you find that the HTMLElement.click() method didn't work on some elements?
I didn't knew it existed. The first SO hit for me was to use the invokeClick, dunno why :/
Rest all of your comments have been implemented.
You already have the
CloseVoteShortcuts.user.js. This new proposed userscript is similar to that, except that it works on all SE sites, and also in all review queues (including the SO-specific Triage, H&I, etc.)This isn't a feature-request though, because I have already made (and tested) this userscript :-) Here is the source code
I have tested this userscript in all review queues each on SO, Math.SE, and Chem.SE. These are the only SE sites where I have 3k+ privileges. However, given that there are entirely no site-specific checks in the entire code, I am bound to believe the userscript would work on all SE site review queues.
I also added an extra feature of being able to go a step back in any popup by pressing the Backspace key. For example, you can directly go from "Close > Off-Topic > Migration" to "Close > Off-Topic" just by pressing a single Backspace, or from "Close > Off-Topic" to "Close", etc.
So, can this userscript replace the one that is currently being used?
What led me to develop this userscript in the first place?
I noticed that once I had started using keyboard shortcuts in the SO close vote queue, my hands naturally wanted to use similar shortcuts in the Suggested Edits/LQP/etc. queues, etc. Moreover, not just on SO, but also on the other SE sites I was active.
At first, I tried to adapt the current userscript to somehow make it work on all SE sites. However I then noticed that every single step of the CV queue process had been hardcoded into the userscript. There was a lot of duplicacy, redundacy, etc. which made it almost impossible for me to be able to extend it in any way possible.
Hence, I came up with this new userscript. The crucial advantage this offers is (apart from working in all review queues) that nothing is hard-coded. So, in case SO later adds/deletes one or more Close options to their Off-Topic dialog, there would be no need to update this userscript at all. Similarly, even if that new Close option happens to be a nested list (clicking it opens a new list), this userscript would still work.
Notes: