Skip to content

Sidebar should not be floating on mobile#1622

Merged
agjohnson merged 1 commit intoreadthedocs:masterfrom
kartben:aside_mobile
Nov 13, 2024
Merged

Sidebar should not be floating on mobile#1622
agjohnson merged 1 commit intoreadthedocs:masterfrom
kartben:aside_mobile

Conversation

@kartben
Copy link
Copy Markdown
Contributor

@kartben kartben commented Oct 18, 2024

When sidebar is set to 100% width on mobile, it should also be switched back to being non-floating.

@kartben
Copy link
Copy Markdown
Contributor Author

kartben commented Oct 18, 2024

The CI error seems unrelated to my change?

@kartben
Copy link
Copy Markdown
Contributor Author

kartben commented Oct 30, 2024

@humitos ping?

@humitos
Copy link
Copy Markdown
Member

humitos commented Oct 31, 2024

I'm not sure to understand what issue this PR solves. Can you expand on that? Maybe attaching a screenshot / video will help here as well.

@kartben
Copy link
Copy Markdown
Contributor Author

kartben commented Oct 31, 2024

@humitos Thanks for the reply!

Basically, if the aside keeps floating when the screen is narrow, the element that follows ends up "swallowing" it, whereas it should instead be back to a "non-floating" layout in this particular situation

screencast.mp4

Depending on how the element that immediately follows the aside is styled, this can cause very visible glitches. It might not be directly reproducible in RTD theme proper, but it can impact people who tweak the theme downstream

For example:

section > p:first-of-type {
    margin-inline: -10px;
    border-inline-start: 5px black solid;
    padding-inline-start: 10px;
}

The incorrect layout would be:

image

While making sure the aside does not float anymore gives:

image

Again, I appreciate that this might not be directly visible/reproducible in RTD theme but IMO it would still be "typographically incorrect" to not implement the fix.

Thanks!

@humitos humitos requested a review from agjohnson October 31, 2024 15:58
When sidebar is set to 100% width on mobile, it should also be switched
back to being non-floating.

Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
@kartben
Copy link
Copy Markdown
Contributor Author

kartben commented Nov 13, 2024

@agjohnson could you help review? thanks!

Copy link
Copy Markdown
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

This looks good to me. 👍 Thanks for the fix @kartben!

@agjohnson
Copy link
Copy Markdown
Collaborator

Not sure what is going on with the test failure, it looks like CircleCI is misbehaving though. I think this is safe to merge.

@agjohnson agjohnson merged commit 8d4d394 into readthedocs:master Nov 13, 2024
@kartben kartben deleted the aside_mobile branch November 14, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants