-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
fix: Website: Make code blocks scrollable on mobile view #1046
Conversation
CLA Assistant Lite bot Thank you for your contribution! Like many free software projects, you must sign our Contributor License Agreement before we can accept your contribution. EDIT: All contributors have signed quick-lint-js' Contributor License Agreement (CLA-v1.md). |
I have read and hereby agree to quick-lint-js' Contributor License Agreement (CLA-v1.md). |
It looks like your solution effectively adds a margin between the right side of the code block and the right edge of the viewport. This tricks the viewer into thinking that the content does not scroll. (There's plenty of space available, so why would it scroll? There's also no scroll bar.) This happens even on desktop: I think it's better to have content which is obviously discoverable rather than pretty-looking content which is harder to discover. I'm open to your solution if the margin is removed or if a scroll bar is visible. EDIT: If you disagree, let me know. |
docs/cli.adoc
Outdated
@@ -306,6 +307,7 @@ ____ | |||
To lint three files, writing machine-readable messages to _/tmp/vim-qflist.json_: | |||
____ | |||
[subs=+quotes] | |||
[source,bash,subs=+quotes,role="scrollable-container"] |
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.
Instead of applying this fix to specific items, could we apply it site-wide? Other pages are affected, such as https://quick-lint-js.com/contribute/create-diagnostic/#report.
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 removed [source,bash,subs=+quotes,role="scrollable-container"]
from the items and added to main.css:
@media screen and (max-width: 670px) {
pre, code {
overflow-x: scroll;
max-height: 400px;
}
}
However i cannot figure out an solution for the scroll bar to always be visible, maybe that would also be browser dependent too.
I do agree on your comment that it's better to have content which is obviously discoverable rather than fancy content which is harder to discover, so given the minor nature of this issue, implementing such a solution may be a bit of an overkill.
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.
(Sorry for the delayed reply.)
I added a screenshot testing tool to the repo. Check out website/backstop/README.md
.
I tried your latest patch on a few browsers. Here's what I saw:
- Firefox Linux: scroll bars only visible when interacting
- Firefox macOS: scroll bars always visible
- Brave Linux: scroll bars always visible
- Brave macOS: scroll bars always visible
- Safari macOS: scroll bars always visible
- Screenshot test (Chromium-based): scroll bars are not visible
This behavior is acceptable. 👍
I noticed a problem on a few browsers: http://127.0.0.1:9001/blog/bug-journey/
With your patch, there is both a horizontal scrollbar and a vertical scrollbar. The horizontal scrollbar is expected, but the vertical scrollbar is surprising. This bug happens with Brave (Linux) and Safari (macOS), but does not happen with Firefox (Linux) or Brave (macOS). This issue might be due to some slight differences in font metrics or something... If there's no quick fix, I'm fine with the behavior of your patch.
Note to self: Looking through a few pages, we really should make some of them more mobile-friendly. There is wasted space in some code blocks due to excessive padding. Your patch makes the problems more obvious (which is a good thing, I think).
website/public/main.css
Outdated
/* Scrollable container */ | ||
@media screen and (max-width: 670px) { | ||
pre, code { | ||
overflow-x: scroll; |
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.
Minor: Is there a reason you chose scroll
instead of auto
? Don't think we want code blocks to have a scroll bar if the code isn't that wide.
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.
Good call, i change it to auto
.
I can't figure out any quick fix for the bug with Brave (Linux) and Safari (macOS), but maybe it is something we could tackle in the future. |
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 PR looks good to me. 👍
One issue: The PR summary has a video of an older version of this PR. Can you update the video, or alternatively remove the videos to prevent confusion?
Okay great, i removed the old videos from this PR. |
This PR addresses the issue of code blocks not properly wrapping on the website in docs/cli on mobile view, which caused a horizontal scroll bar to appear on the bottom of the screen and the actual code block content overflowing.
As a solution i updated the AsciiDoc to apply a custom CSS class to code blocks and added a CSS rule to make any element with the new class horizontally scrollable, while preserving spacing and line breaks.