fix(TOC): fixes toc showing incorrectly at exactly 1450px#4802
Conversation
rebeccaalpert
left a comment
There was a problem hiding this comment.
The table of contents spacing looks different to me here, but I am on a smaller screen, so take this with a grain of salt:
| 1451 | 1450 |
|---|---|
![]() |
![]() |
I think this was what Parthiv's margin stuff was supposed to fix.
I'm wondering if we don't need to address https://github.com/patternfly/patternfly-org/blob/main/packages/documentation-framework/components/tableOfContents/tableOfContents.js#L99 and/or https://github.com/patternfly/patternfly-org/pull/4802/files#diff-2bc5461d65edc211f100c8082b2a58df1044e3f682759af7a27228f7c94288c1L75 as well if we are doing this. Probably the min-width needs to be adjusted and we probably need a >= in the .js. I haven't tried this, but I suspect that's relevant as well.
|
@rebeccaalpert You were right about the other media query; that fixed the difference in spacing. All the numbers were off by 1px and that's based on our 2xl breakpoint because the jumplinks have the I also adjusted the offset to add a condition because our page also reconfigures slightly at the md breakpoint of 768px, so we needed a different offset value between 768 and 1450. Finally, @lboehling I tightened up the spacing around the TOC toggle from a -md block padding to a -sm because it seemed so dang big ;-) Happy to undo that if you don't like it! One last comment..the background color on the mobile version makes it appear that you can click anywhere, but you can really only click on the button, which is only as long as the text. I toyed with making the button full width or turning off the background but left it as is, so if you think it would be good to change that and have ideas, lmk! |
rebeccaalpert
left a comment
There was a problem hiding this comment.
Looking good; thank you!
My only comment is a style thing - technically it works. On this line, it's probably better to break up the nested ternary operator: https://github.com/patternfly/patternfly-org/pull/4802/files#diff-150f1ee54d93b6a1323d99e56a2963cf63ab097113bf89e9d6eac67a90d787f1R99
It's just harder to read. Typically I'll see nested one-liners like this reworked into a switch statement or a function (getOffset or similar).
mcoker
left a comment
There was a problem hiding this comment.
Mentioned offline but looks like there is still a ~11px or so viewport width where the TOC drops below the main content. I'm thinking it might have to do with the TOC width
and the content area max-width
patternfly-org/packages/documentation-framework/templates/mdx.css
Lines 27 to 29 in 33f34db
and the flex wrapper for them, which will wrap by default
patternfly-org/packages/documentation-framework/templates/mdx.js
Lines 111 to 113 in 33f34db
I'm wondering if maybe we should add .pf-m-nowrap to that flex layout to keep them from wrapping when they're in a row, and .pf-m-row-on-2xl to switch the layout to a row at the same point the TOC/jump links switches from mobile to desktop?


Fixes #4747
This fixes the small blip where the menu shows incorrectly at exactly 1450px.
One known problem: if the TOC is expanded when in the dropdown (mobile) mode, then the
.pf-m-expandedclass will be added. If the window is resized to be larger than 1450px, the TOC will be displayed vertically - however, that class is never removed. So the next time the user resizes the window to be smaller than 1450px, the expanded class is still there and the menu will show as expanded.