Skip to content

Increase page max width to show more content when possible #5523

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

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented May 3, 2025

This aims to increase the page's maximum width so that it can show more content when possible.

From what I could tell, the current max width is 800px. It's a bit funny to have to scroll horizontally for somewhat-long commands when there's plenty of space, e.g., on an ultrawide 3840×1600 monitor:

image

This PR adds a custom CSS file that sets the max width to 1000px. The page still correctly adapts if the width is less than that, and this allows us to see more content without having to scroll horizontally.

image


I found this relevant issue/discussion: readthedocs/sphinx_rtd_theme#295. There are some valid points against increasing the max width. One of them is that a wider page might decrease reading comprehension: readthedocs/sphinx_rtd_theme#295 (comment). I think this is true, but increasing the width from 800px to 1000px might not be that bad and could help a lot.

It could break things that rely on a specific width, but I've taken a quick look and don't see anything obviously broken.

I opened this as a prototype so that we could discuss it. We could make it bigger if we want, e.g., 1200px, but that might be too much. I thought this might've been discussed before (I sort of vaguely remember something about increasing the page width), but I couldn't find any issue here.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard added the backport-all backport at reviewers discretion; from rolling to all versions label May 3, 2025
Copy link

github-actions bot commented May 3, 2025

HTML artifacts: https://github.com/ros2/ros2_documentation/actions/runs/14839938935/artifacts/3063058012.

To view the resulting site:

  1. Click on the above link to download the artifacts archive
  2. Extract it
  3. Open html-artifacts-5523/index.html in your favorite browser

@@ -0,0 +1,3 @@
.wy-nav-content {
max-width: 1000px !important;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the !important strictly necessary or can we find a way to do this with correct CSS precedence? Using !important competes with userstyles that are doing exactly similar things in user stylesheets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used/kept it because of this comment, which does mention that styles may not all play nice depending on the extension: readthedocs/sphinx_rtd_theme#295 (comment). I removed it and quickly looked through some pages, and I don't see anything wrong, so let's do that. We can always reevaluate later if we find actual issues.

@@ -0,0 +1,3 @@
.wy-nav-content {
max-width: 1000px !important;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
max-width: 1000px !important;
max-width: 64rem !important;

Using rem instead of pixel values will scale the viewport with font-size. So if someone increases the font size the viewport will expand to accommodate it. 64rem * 16pt = 1024px.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no CSS expert, so thanks for the info! Proper scaling was also discussed in readthedocs/sphinx_rtd_theme#295. That makes sense.

One could say, it's not very... important.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this increases responsiveness on devices with varying screen resolutions and user settings for font size, and flexibility making the layout more scalable to changes in user preferences or browser zoom levels.

i am not CSS expert... maybe we can merge this with @nuclearsandwich 's approval?

@christophebedard
Copy link
Member Author

I was honestly expecting more reticence about the actual width increase part:

There are some valid points against increasing the max width. One of them is that a wider page might decrease reading comprehension: readthedocs/sphinx_rtd_theme#295 (comment). I think this is true, but increasing the width from 800px to 1000px might not be that bad and could help a lot.

but I'm happy if people think this is a good idea.

@christophebedard
Copy link
Member Author

One question: Should we only do this with the Rolling docs for now? Then, if we haven't found any issues after some time, we can backport this to other branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-all backport at reviewers discretion; from rolling to all versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants