-
Notifications
You must be signed in to change notification settings - Fork 391
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
Mobile quality-of-life (part 1) #2013
Mobile quality-of-life (part 1) #2013
Conversation
On small screens, the page was scrolling horizontally because the cell actions were too wide and the hidden tooltips extended beyond the page. This commit reduces the spacing between icons on smaller screens and hides the tooltips since they're not accessible through mobile touch anyways. This commit partly addresses livebook-dev#2011.
Addresses part of livebook-dev#2011.
On smaller screens, there isn't enough room to render the cell insert buttons between the editor controls; they overlap, making it difficult to read and click. For now, this commit renders cell insert buttons on their own line (and fixes some positioning issues). A nicer fix might be to use a custom/different menu on small screens that collapses all controls into a toggleable menu. Addresses part of livebook-dev#2011.
This commit also introduces a function for updating the editor options based on the size of the current screen. It's called when the existing `ResizeObserver` fires, so the options should be reactive to screen size changes (e.g. portrait to landscape mode). Addresses part of livebook-dev#2011.
There is unfortunately not a great way to do this. Apparently, you can increase the textarea font-size to 16px, but I don't think we want to do that. Adding `maximum-scale=1.0` to the viewport meta tag works well in WebKit browsers, but some mobile browsers, like Chrome on Android, will prevent the user from zooming at all if that is present. Instead of doing that, this hook does its best to detect that we're in a WebKit browser and only adds the meta attribute if so. Addresses part of livebook-dev#2011.
Addresses part of livebook-dev#2011.
Uffizzi Preview |
1afcc03
to
872dfb8
Compare
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.
@zachallaun awesome, just a couple comments!
assets/css/tooltips.css
Outdated
@@ -31,7 +31,7 @@ Example usage: | |||
content: attr(data-tooltip); | |||
white-space: pre; | |||
text-align: center; | |||
display: block; | |||
display: none; |
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.
Hmm, tooltips can actually be accessed, so we should keep them.
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.
Hmm, I wasn't ever able to access them on my iPhone. Perhaps there's a gesture that I'm not familiar with to trigger a hover?
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.
I tried in Safari on mobile and holding an icon shows the tooltip for me.
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.
Okay, you’re probably right! I think it’s an OS setting — on my phone, holding shows a native pop-up to open in a new tab/etc.
Do you have suggestions for how to handle the tooltip extending past the page on some screens and causing a horizontal scroll? I remember playing with overflow settings on containers, but ended up with other undesirable behavior.
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.
Okay, so I think I'm going to remove this from this PR and let it just have a tiny bit of horizontal scroll, then I'll address it in another PR.
I think the solution for this, and possibly for menus as well, is a hook that looks at the getBoundingClientRect
and compares that to the viewport dimensions, shifting the element into view if required. I don't want to include that here, though.
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.
Yeah, having a hook for every tooltip feels kinda weird, but I'm not sure. I like how the current solution is just CSS, but it does have a limitation if the tooltip doesn't fit. Perhaps we could have a single global listener for mouseenter/mouseleave and implement tooltip positioning more dynamically with 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.
Yeah, that would be kinda weird. There's a CSS solution that's coming at some point (no idea on the timeline). It's a small enough issue that it's probably worth just waiting for 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.
Oh, this looks great, it would be super useful!
Thanks for the review @jonatanklosko! With the exception of the few things I commented on, these suggestions should be implemented. :) |
@jonatanklosko All comments should now be handled! |
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.
Thanks!!
This PR implements/fixes the bulk of #2011.
As much as possible, commits in the PR are atomic and can be reviewed in order if that's easier. Most of the commits have additional context as well.