-
Notifications
You must be signed in to change notification settings - Fork 4
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
Apply figure padding only when top level of content #1190
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1190 +/- ##
==========================================
- Coverage 36.45% 36.43% -0.02%
==========================================
Files 435 435
Lines 19288 19306 +18
Branches 6330 5700 -630
==========================================
+ Hits 7032 7035 +3
- Misses 12218 12233 +15
Partials 38 38 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing seems to be thoroughly breaking here, but I've noticed a slight discrepancy with figures in the pre-amble content before a main tabs/accordion/etc. section. I'll discuss with pictures in DMs.
(essentially, the images out of an accordion are out-of-line with the text, and the images in an accordion are in-line with the text as expected - so its inconsistent).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this is causing some inconsistencies (different ones this time) :/ On pages like Phospholipids (Part B), Figure 1 is now not being indented even though it should be as content inside the accordion.
It does not follow the provided css structure of content-chunk > ... > content-chunk > figure-panel
.
src/scss/phy/questions.scss
Outdated
background: none; | ||
box-shadow: none; | ||
padding: 0 0.25rem; | ||
font-weight: 400; | ||
} | ||
|
||
.content-chunk .content-chunk > .figure-panel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best way to do this is to directly check for the page's top-level class (which all preamble content is a child to) and not indenting in that case. This seems to produce all the changes with correct indentation.
.content-chunk .content-chunk > .figure-panel { | |
:not(.generic-panel):not(.concept-panel):not(.question-panel) > .content-chunk > .figure-panel { |
(alongside adding the classnames concept-panel here and generic-panel here to mirror the structure of question pages)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In truth, I'm not a fan of this approach in general. It isn't future-proof to more page types, the CSS is increasingly less legible with each fix we make to when it applies, and relies on the structure of pages not to change. Currently, if we add a new layout type, we might, depending on whether this component wraps its children in an arbitrary container after the outer content-chunk
, add the padding to figures inside. Does this make sense? No. It should either apply to all children nonprejudicially, or none. To put it another way: why does content inside a layout block (e.g. accordion) act differently to if it was outside this block, besides being displayed in a smaller box?
I think we have unfortunately stumbled into a questionable decision made a while ago. Cleaning this up will have even more widespread implications than this already has. 😮💨
With the redesign approaching, and given your solution is only a couple more patches, let's just go with it. I'll make a point to tidy it up when we overhaul content layout though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this still doesn't work for expansion layouts (see /questions/97c57c26-357e-4ae1-a21e-820778a9993d
) -- we don't want padding there. We can't add another :not
to the panel level becauseexpansion-layout
is applied much further up in the DOM. I can't immediately see a patch for this. Component-level padding looks like such an oasis...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in concept. We really ought to just let the container do the indenting and be done with it 😔. I was trying to find the least structurally dependent solution that actually Works™ for now.
In terms of expansion layouts, I suppose we can safely say that including other structure components (like another accordion) inside an expansion-layout
is bad behaviour that shouldn't be done and would probably lead to more bugs anyway. In which case, figures never need to be indented and so adding another extra css rule along the lines of this should do it?:
.expansion-layout .content-chunk > .figure-panel {
padding: 0 !important;
}
(OR like I said in-person, I'm happy to just ignore the edge case and accept it as slightly off)
Frankly, this was meant to be a small fix and it's clearly a bigger problem. I'm probably happy with however you might want to resolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another accordion probably would be bad behaviour, but a side-by-side or custom-offset figure isn't out the question. Let's leave as is for now; I've made a card to solve this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A side-by-side shouldn't need any padding anyway (hence the original problem), but okay yeah this is fine. We can handle it better separately and it's already quite niche.
This, while simple in concept, affects every figure on the site so should receive fairly heavy testing.