Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1995 +/- ##
==========================================
- Coverage 43.17% 43.00% -0.17%
==========================================
Files 575 576 +1
Lines 24620 24631 +11
Branches 8128 7281 -847
==========================================
- Hits 10630 10593 -37
- Misses 13322 13989 +667
+ Partials 668 49 -619 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ntainer-refactor [VRT] Update baselines for improvement/sidebar-container-refactor
sjd210
left a comment
There was a problem hiding this comment.
I haven't found any issues with sidebars appearing where they shouldn't/not appearing where they should which is the main thing I was concerned might come up (including when testing using the My Ada sidebar branch to see it in action), but I do still have some thoughts.
| const { className, show=true, ...rest } = props; | ||
| return show | ||
| ? <SidebarContext.Provider value={{sidebarPresent: true}}> | ||
| <div {...rest} className={classNames("d-flex flex-column sidebar-layout", siteSpecific("flex-lg-row", "flex-md-row"), className)}/> |
There was a problem hiding this comment.
This className (rather than the simpler "sidebar-layout row" of before) is shifting everything on Sci horizontally such that sidebar and main content and now much closer together, the sidebar is smaller, and it doesn't line up as well with the left side of the page (i.e. aligning with the title hex icon/my isaac/logo). I imagine it isn't intentional, but regardless I don't like it.
I'm also not entirely clear on what the class is meant to achieve. It seems to me from quickly looking at the My Ada branch that the "d-flex flex-column flex-md-row" is only needed for Ada and not Sci, since we use things like <Col ... xs={12} lg={8} on L32 to handle this kind of stacking behaviour on Sci. I think that this dichotomy is for the sake of sidebars being a fixed size on Ada, but a proportion of screen-size on Sci, but I'm not fully sure?
An easy solution is just to move the new classNames inside the Ada branch of siteSpecific, but it would be nicer for the content/sidebar stacking to work in a similar way across both sites, if at all possible.
There was a problem hiding this comment.
I think I did this because we weren't really using row properly before. Even though it ended up looking fine, the row has negative margin (what with being a row and this being Bootstrap), but this does not match the parent .container's padding.
These two images are identical besides the box model, so you can see that the sidebar's padding overlaps the page container's padding badly:
I suppose this would have meant that a new, fixed-width sidebar doesn't take up the width it claims to take up, which might have been why I changed it; I can't remember for certain though. Alternatively, this could have been solved by adding g-0 to give the same outcome while keeping the same sidebar-layout row structure, though all the changes below this point would still be required, and there might well have been reasons I am forgetting.
You are right, though, that the new approach ends up looking bad on Sci – but this is because the p-4 class that the sidebar has is now actually insetting on the left, rather than arbitrarily being shifted by -1.5rem:
(aligned, at least!)
I much prefer this structure; we can then just kill off some of the left padding to restore it to its former glory (ps-1 most closely matches the title hexagon's spacing, but ps-3 most closely matches the old layout):

The only real difference left is that the row approach enforces spacing between the sidebar and the main content, which this new approach no longer does. I'd probably use gap in any other situation, but for Sci the sidebar:content ratio is fixed at either 1:2 or 1:3, and gap adds extra that causes overflow. I think my preferred alternative is just to increase the padding on the right of the sidebar, something like pe-5; this leaves the <MainContent> flush with the actual contents.
I'll make a commit with these changes on (appreciate long paragraphs are probably a bit hard to follow!) and would appreciate any thoughts from there.
There was a problem hiding this comment.
it would be nicer for the content/sidebar stacking to work in a similar way across both sites, if at all possible.
As you mention before, this is quite difficult owing to the relative positioning of the sidebar and main content given the requirement for the Ada sidebar to exist flush with the left of the page. One possible improvement might be to replace <MainContent>'s <Col> lg / md sizings on L32 with just a div with flex-grow-1, which is how the Ada one should work, so that component is the same across both. But this is only possible because the sidebar itself has a fixed lg / md size on Sci, so the automatic sizing always gives (100% - sidebar%), which is precisely the sizings we would be replacing.
Co-authored-by: Sol <94075844+sjd210@users.noreply.github.com>
I've kept a small bit of margin as there are some pages without any at all that look odd (esp. Ada).
Refactors all uses of
<SidebarLayout>that are directly underneath a<Container>into a new layout component,<PageContainer>. I intend this to supersede the old sidebar layout going forwards, providing consistent spacing and easier modifications to site-specific layouts.Also allows (but does not provide examples of) sidebars to exist on Ada, by removing the
isAdachecks in the two main sidebar types. There are a couple of instances where we used a<SidebarLayout>well away from its parent<Container>meaning they cannot easily be merged into the new component – to keep these components from appearing on Ada, we also introduce ashowprop on<SidebarLayout>that can achieve the same effect. This ought to be used minimally.I would strongly recommend you view changes with whitespace disabled! Most pages' `{body} elements (see below code) have been unindented twice.
Old (Sci):
"Old" (Ada) – this was never live, but had existed on Ada sidebar PR for a while:
The discrepancy between Sci and Ada is owing to the Ada sidebar being flush with the left side of the screen, while the Sci sidebar has padding to the left and exists below the page title.
New (both)
The same layouts as before are still generated as the result of the
<PageContainer>component, but the internal layout and site-specific adjustments are all now abstracted.