a quick few changes, not sure if you'll find them appropriate :) #44
Conversation
mouton-rebelle
commented
May 7, 2013
mouton-rebelle
commented
- next and previous shortcut now toggle story
- when a story is toggled, screen is scrolled
- added b as a shortcut to open permalink to match Reeder shortcuts
- permalink now targets _blank
- next and previous shortcut now toggle story - when a story is toggled, screen is scrolled - added b as a shortcut to open permalink to match Reeder shortcuts - permalink now targets _blank
|
Really love this project, this pull request is just to match my personal taste / habit, don't hesitate to reject it :) |
Got to pack some OS detection for guessing ctrl/cmd key
app/public/js/app.js
Outdated
There was a problem hiding this comment.
the thirteenth parameter of initMouseEvent sets meta key
|
I am good with the first 3, need to mull over the _target=blank stuff. I like the new tab behavior, but am not sure about the approach with the meta/ctrl key stuff. |
|
I think I'd prefer to just use window.open() and let the user's browser settings determine if it is in a new tab. |
|
The target blank on the link is not necessary, it can be remove. But there is no other way than the meta to open a link in background. |
|
Ah - so the open in the background part is the reason for the ctrl/meta. I'm okay with opening in the foreground in a new tab with _target=blank. |
|
+1 for the changes, -1 for the OS checks and Ctrl/Meta stuff, +1 for window.open(). |
Permalink has no target (html) When key [b] or [v] is pressed, it opens in a new window (tab) in foreground.
|
okay, followed your advice and removed the trick :) will keep it for myself though, I feel the key shortcut is useless if you can't open in background. It's a matter of taste I guess, I like to go through my whole rss feeds, open a bunch of new tabs for stories that looks promising, and read them after. |
|
Thanks - will try to merge this tonight. |
