-
Notifications
You must be signed in to change notification settings - Fork 29
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
Switch to Canvas Rendering #857
Conversation
cdf260d
to
1b9916f
Compare
We probably don't need the rendering tests anymore, now that livesplit-core is handling it |
I'll test out the changes in this PR later today and see if I notice any issues. |
It's still better to keep the tests I think, since that ensures the HTML canvas also behaves the way we expect it to, since that's not something lsc tests. |
This switches the rendering of the layout from using HTML and CSS to using a canvas (technically it's two). This not only should improve the performance but also brings the rendering in line with our native renderers, enabling new features such as vertical resizing and horizontal layouts. This also removes a lot of code specific to the HTML rendering and thus greatly simplifies the codebase. There is one slight regression in that the layout editor doesn't allow clicking and dragging the individual components directly on the layout anymore. This is however something we can explore directly in `livesplit-core` in the future, so other frontends can benefit from it as well.
Alright the tests pass now. The only thing that I know that's still left to fix is the "mobile layout" where we use 100% width, but I forgot doing the same for the height... though it's more complicated there, because we also want the control buttons underneath, so like 100% - the height of the buttons. It's pretty late here, so if you want to fix that up, I'd appreciate that. |
One thing that's not ideal right now is that layouts don't auto save. So if you change the layout direction but don't click save and then refresh, it remembers the size but not the direction, and ends up in a weird state |
Yeah I noticed that too. I think that'll be resolved once we save the size in the layout itself, so it's probably something we can (should) live with for a bit. |
Prompting for unsaved layout changes would be helpful, too. I think that'll need to be something built into livesplit-core? |
This logic makes sense for a vertical layout, but probably not for a horizontal layout. I think a horizontal layout shouldn't take up the entire height, and it should go off the edge of the screen if necessary (since it's hard to fit an entire horizontal layout into a narrow width) |
Yeah but you probably just don't want a horizontal layout on mobile anyway. But yeah filling the entire screen is probably not ideal always anyway. I think these can all be follow up issues. |
This was causing the layout settings to not be interactable, since the resizing element was blocking them.
I pushed a change to auto adjust the layout height on mobile, only when the layout direction is vertical. (Feel free to squash my commits - I just don't like force pushes when it's not my PR.) (The horizontal layout still doesn't work very well on mobile, because it requires a ton of horizontal scrolling and also makes the editor views super wide. But that's a problem to fix later.) |
No need, as long as the commits are clean(ish), they are totally fine. |
This switches the rendering of the layout from using HTML and CSS to using a canvas (technically it's two). This not only should improve the performance but also brings the rendering in line with our native renderers, enabling new features such as vertical resizing and horizontal layouts. This also removes a lot of code specific to the HTML rendering and thus greatly simplifies the codebase. There is one slight regression in that the layout editor doesn't allow clicking and dragging the individual components directly on the layout anymore. This is however something we can explore directly in
livesplit-core
in the future, so other frontends can benefit from it as well.