-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add fullscreen screenshot view at app pages #494
Conversation
This should be debated, but maybe you can try to implement this: https://stackoverflow.com/questions/2153979/chrome-extension-how-to-save-a-file-on-disk |
Imho, for "Download full-size" link it is not necessarily. Opening full-size image in new tab is pretty enough and allows to check out that full-size before actual writing on disk. In case you will be forced to download full-size image first, you will need to open it in your OS to check out. Also, such things like forcing to download instead of opening in browser, is usually done by server-side, by passing special headers. |
Yes, I know this. But the discussion I linked offered other solutions to that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very clean, but we have a (very vague, still undocumented) style guide that you couldn't know of.
Shouldn't be hard to adjust though.
Also, I'm not sure whether the keyframe hook is required
js/content/store.js
Outdated
modalFooter.appendChild(downloadButton); | ||
} | ||
|
||
document.addEventListener("animationstart", es_initFSVButton); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just listen to body
's changes with a MutationObserver
with the parameters { childList: true }
?
Because the code doesn't require the modal to be fully opaque?
Correct me if I'm wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean with being fully opaque, but listening to an event seems less intrusive than observing the document body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I really don't remember why I used the hook in this case instead of the observer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of actually making overhaul of screenshot viewer there is much better way to deal with it. Replace whole screenshot modal popup by replacing old and ugly function that creates it. Here:
HighlightPlayer.prototype.ShowScreenshotPopup = function( screenshotid )
at
https://steamstore-a.akamaihd.net/public/javascript/gamehighlightplayer.js (:555)
Simple button which activates fullscreen mode to comfortably view screenshots at store pages.
Description:
Function from those which Valve should implement by themselfs long ago. Personally I use this feature for about 2 years now (as self-written userscript) and cannot imagine Steam-shoping without this. Because how else you can checkout how these games will look like at your own screen? Well, to be honest Steam client's Big Pucture can show store screenshots at fullscreen. But we are advanced users who use all those advanced functions in browser and we need those screenshots at fullscreen right here right now. There they are, right in this pull request.
Wow! Felt like a marketer by writing this.
Core features:
Contained JS is necesarily only for including needed buttons and activating browsers built-in fullscreen mode. Whole hard work with images is done by native Steam's UI, for which was created fullscreen design by using CSS!
Try out:
You can try basic implementation of it by installing userscript from here:
https://gist.github.com/thomas-ashcraft/00e58a0141fda12199d5e1fdee821ecf
Code is year old, partially updated just yesterday to avoid use of jQuery. But its universal and can run in almost any modern browser (except IE), even not updated ones for last few years.
Technical notes:
Was tested in Firefox and Chrome. Working like a charm even at low RAM, bad connection, and awfully loaded 6 years old CPU.
My original pull request from may 2018 (jshackles/Enhanced_Steam#1584) didn't merged because Enhanced Steam was in fact already dead back then.
At those moment major browsers still not yet completed Fullscreen API implementation. Which led to using dirty ways: additional hooks, additional JS code, or tonns of additional redudant CSS. Now all fullscreen requests and styling can be done easily by well documented cross-browser ways. At least for Firefox and Chrome.
So current brand new implementation of Steam screenshots fullscreen view contains only 2 JS dirty tricks.
Those things can be theoretically avoided by complete rewriting Steam's internal function for screenshot_popup_modal. (
HighlightPlayer.prototype.ShowScreenshotPopup
)The second one trick can be partially avoided by a lot of CSS. And nevertheless demands JS intervention for at least proper positioning.
There is still a room for improvements. For examples:
Pass thumbnails line to modal and/or fullscreen mode.
Make option for using fullscreen mode by default.
Place fullscreen view button right at app page, not just at modal popup.