Skip to content
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

Updated layout to adjust width on xl screens #232

Conversation

Theresa-o
Copy link

Updated layout to adjust width on xl screens
Fixes #223
Adjusted the width for larger screens using bootstrap classes "col-xl-2" "col-xl-2" to remove the extra width

@Scotchester
Copy link
Contributor

Hi Theresa,

Could you please provide some before and after screenshots?

@Theresa-o
Copy link
Author

Ok, let me do that now.

Thanks.

@Theresa-o
Copy link
Author

Theresa-o commented Oct 24, 2022

This is the site with extra spacing at the edge

wagdocsxlnotadjusted

This is the site after the bootstrap classes have been added to adjust the large screen width

wagdocsxladjusted

Please let me know if this is what you meant and if this would suffice

@Scotchester
Copy link
Contributor

Yes, that is what I was wanting to see, thank you! I'm afraid that your change doesn't seem to be addressing the crux of the issue, though.

The space being present is not the problem itself. The real problem is that the page-level table of contents (the right sidebar) can sometimes have a horizontal (left-right) scrollbar, as noted in the original issue:

The main content container and the page TOC seem to have some limited width set somewhere. This is particularly jarring since it creates horizontal scrolling on the page TOC 😬

If you load the page in the original issue screenshot, you can see this horizontal scrolling.

Adding with to the main content column will not fix that – in fact, it is probably already too wide with respect to best practices for readable line lengths – we just want to eliminate those horizontal scrollbars.

That said, the horizontal scrollbars should not be there, regardless of the browser width. Try reducing your browser width to a size where the text will wrap to see this. (I see it at 1920px wide.) The text in the page TOC will wrap, and yet the scrollbars are still present, so adding width to the page TOC column will probably not solve the issue, either.

@Theresa-o, could you try to diagnose the root cause of the horizontal scrolling and fix that?

@Theresa-o
Copy link
Author

Theresa-o commented Oct 24, 2022

Ok, I would confirm this and provide update

@Theresa-o, could you try to diagnose the root cause of the horizontal scrolling and fix that?

@lb-
Copy link
Member

lb- commented Oct 25, 2022

See also @Scotchester some improvements to the horizontal scrollbar issue in #215 (essentially forcing the content to wrap instead of adding scrollbars.

@Theresa-o
Copy link
Author

@Scotchester @lb- The horizontal scrollbar is still persistent because currently the default state is overriding 'overflow-x: hidden' so when inspecting the page 'overflow-x: hidden' is not showing as seen in the below image

wagdocscrollbarhiddeninspect

As a result, we would need to bulldoze through specificity and use !important to ensure that the browser implements this.

I'd like to make a PR with the following code changes, please let me know your thoughts/suggestions

.page-toc {
//updated code
overflow-x: hidden !important;
max-width: 100%;

//old code
font-size: ($font-size-base * 0.875);
padding-top: 2.4rem;
color: $gray-700;
max-height: 100vh;
overflow-y: auto;
word-break: break-all;
}

Please see screenshot of the effect of the updated code below;

wagdocscrollbarshown

@lb- lb- requested a review from Scotchester October 29, 2022 05:30
@Scotchester
Copy link
Contributor

@Theresa-o My sincere apologies – I overlooked the fact that the horizontal scrollbar problem I was talking about was already fixed in PR #215 that @lb- mentioned. This solves the most acute symptom described in issue #223 that you were trying to address, but I do think there are still some improvements to be gained in how our columns lay out over the range of screen sizes. I think it is more nuanced than simply widening the main column as you originally proposed in this PR, though.

I'm going to close this PR and comment back on #223 with my thoughts on the remaining problems.

@Theresa-o
Copy link
Author

Theresa-o commented Nov 17, 2022

No problem, okay @Scotchester

@thibaudcolas thibaudcolas added the Outreachy https://www.outreachy.org/ label Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Outreachy https://www.outreachy.org/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On large screens full width is not used, but page TOC need horizontal scrolling
4 participants