Make for smooth vertical resizing of remote screen#1583
Merged
jotaen4tinypilot merged 1 commit intomasterfrom Aug 23, 2023
Merged
Make for smooth vertical resizing of remote screen#1583jotaen4tinypilot merged 1 commit intomasterfrom
jotaen4tinypilot merged 1 commit intomasterfrom
Conversation
jdeanwallace
approved these changes
Aug 23, 2023
Contributor
jdeanwallace
left a comment
There was a problem hiding this comment.
Automated comment from CodeApprove ➜
Nice, LGTM! I've tested on device.
I discovered tinypilot#1581 while testing this PR; so this behaviour is not a regression, but it’s currently broken on master.
Oh wow. For me in Chrome, I experience this bug only when I'm in full screen mode and the browser's dev console is closed.
👀 @jotaen4tinypilot it's your turn please take a look
This was referenced Aug 23, 2023
jotaen4tinypilot
added a commit
that referenced
this pull request
Aug 24, 2023
Part of #728, follow up of #1583. Non-functional refactoring. #1583 left a stray `<div>` in the `index.html` file, which doesn’t serve any purpose anymore. This PR removes the `<div>`, and lifts up the `<remote-screen>` and `<on-screen-keyboard>` components. That doesn’t have any actual effect, as all HTML elements are positioned absolutely anyway, but to me it makes for a slightly more intuitive code structure: i.e., we instantiate the basic elements in the order in which they appear on the screen, and have all dialog elements underneath. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1586"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
jotaen4tinypilot
added a commit
that referenced
this pull request
Aug 24, 2023
Part of #728, stacked onto #1586. As demonstrated and discussed [in the original proof-of-concept branch](#1574), we want to hide the status bar and menu bar in the popup window [via a query parameter](https://github.com/tiny-pilot/tinypilot/pull/1574/files#diff-66e482e36d313dd641cd92c815c5fa30fb380bbaa601810827b30a5b84428856R351). So by appending `?viewMode=standalone` to the TinyPilot URL, this PR hides the status bar, the menu bar, and the on-screen keyboard. We will use this in a later PR, when adding the menu item for opening a “real” popup window. <img width="783" alt="Screenshot 2023-08-23 at 18 38 09" src="https://github.com/tiny-pilot/tinypilot/assets/83721279/c3a22b9b-b3b8-49a3-a5b1-88d2368337ec"> The query parameter can also be used independent of the popup window, which e.g. allows for [the iframe use-case](#523). ## Notes - `prettier-ignore` is needed above `<remote-screen>`, since Prettier cannot handle Jinja2 templates, and would enforce a pretty [bulky formatting otherwise](https://github.com/tiny-pilot/tinypilot/pull/1574/files#diff-a18b3b5de30df1bcf7531723d24c214d69b2acff3cd88540e1ff186409879b0aR81-R88). - I had to pull out a `--bar-padding` CSS variable in the `<remote-screen>` component, so that we can dynamically override the value. I forgot that in the [earlier PR](#1583). - In `index.html`, I injected the `display: none` via a plain `<style>` block to hide the elements from the page. Since `index.html` itself isn’t a web component, I’m not sure we can mimic our usual pattern with HTML attributes here ([example](https://github.com/tiny-pilot/tinypilot/blob/aa80dbdd3e58e79401008665f2199d98fa14b3d0/app/templates/custom-elements/change-hostname-dialog.html#L12-L14)), at least not without introducing more complexity and indirection. The `<style>` block is not super beautiful, but I thought it’s simple and straightforward at least. - Note sure `standalone` is the greatest name for the query parameter value. Potential alternatives: “screen-only”, “focus-mode”, “dedicated-screen”. (It’s maybe not *that* critical, however… 🤔) In any event, I found something like `?viewMode=standalone` more descriptive and flexible than `standaloneMode=true`. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1587"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Part of #728.
The vertical resizing behaviour of the remote screen is currently so that the screen element’s height appears to “jump” at certain steps due to CSS breakpoints, while you resize the window vertically.
This PR implements “smooth” and step-less vertical resizing, while maintaining a constant margin between remote screen and status-bar during that process.
The breakpoint-less resizing is important for the “dedicated window” (popup) that we are about to implement in #728, where it would probably feel weird if the remote screen height jumped around the way it currently does, or if we wouldn’t be able to fill up the entire vertical space of the popup window due to the breakpoint setup.
Demo: After
Screen.Recording.2023-08-23.at.13.52.12.mov
Demo: Before
Screen.Recording.2023-08-23.at.13.51.32.mov
Notes
<div>inindex.htmlwhich we technically don’t need anymore. I’ll clean that up in a later PR, to avoid a noisy whitespace diff here.page-contentCSS class is orphaned, so I’ve removed it.I’ve tested on device with Firefox and Chrome, and I tried to check all imaginable scaling constellations of window sizes and aspect ratios, both with MJPEG and H.264, and both in full screen and regular window mode. The biggest risk of this PR would be that we accidentally mess with the mouse cursor positioning/alignment. It would be cool if you could double-check on device as well, with a real target screen, just so that we are safe.
