Notebook layouting compatibility with high DPI / widescreen displays #2098 #2543
Conversation
janno42
requested review from
hansegucker,
janno42,
niklasmohrin and
richardebeling
| max-width: 75vw; | ||
| margin-left: 25vw; | ||
| .notebook-card { | ||
| height: calc((100vh - #{$navbar-brand-height}) - (#{$footer-height} + ((#{$spacer} * 1.5) * 5))); // subtract 3 spacer with padding |
There was a problem hiding this comment.
Comment does not match computation
I can visualize 100vh - navbar-height - footer-height, and I would understand subtracting 2 spacer heights, but the factors 1.5 and 5 are a bit unclear to me here
There was a problem hiding this comment.
Tried the equation both with 2 spacer heights and the documented 3 spacer heights, it looked wrong. My "solution" now is to change the comment to an educated guess of the purpose.
There was a problem hiding this comment.
I just now saw that this line was already in the CSS and you just moved it -- GitHub just happens to show this as an added line first. It would be nice to come up with a good explanation of why this makes sense, or finding a better value if it doesn't, but it's not required from you here.
There was a problem hiding this comment.
//approximation for similar gap to the footer as evap-Content
is my current understanding of the formula
| display: flex; | ||
| justify-content: center; |
There was a problem hiding this comment.
I feel like we should put direct attributes at the top or bottom, not somewhere in the middle between nested selector blocks (or whatever they are called).
Why do we need these now, if we were happy without them before? This is required to center the top-bottom notebook, right?
@niklasmohrin @janno42 any guidance for "member" ordering here?
There was a problem hiding this comment.
intuitively I would also say that nested blocks should come only after all declarations for the parent, but I will take whatever makes the most natural diff for this PR; currently it looks like things were moved around and I would appreciate if stuff would stay mostly where it is :D
There was a problem hiding this comment.
I would expect to find these attributes at the top
There was a problem hiding this comment.
Why do we need these now, if we were happy without them before? This is required to center the top-bottom notebook, right?
This centering is necessary to emulate the look of the previous not width-limited top-bottom notebook, since it used to fill out the screen and was centered implicitly.
There was a problem hiding this comment.
necessary to emulate the look of the previous not width-limited top-bottom notebook
okay, but then it should be fine to have them inside the corresponding css media selector, right? I feel like structuring the CSS here like that could possibly help having a good overview of both modes -- but following an approach of getting a minimal diff is probably also fine, but if you need top-level CSS styles, please put them at the top.
| display: flex; | ||
| justify-content: center; | ||
| .notebook-container { | ||
| width: 80vw; //approximation to be similar to evap-Content width |
There was a problem hiding this comment.
Can we use the exact same value as we use with evap-content? I feel like there should be a way to factor that out into a variable and reuse it here.
There was a problem hiding this comment.
The exact value would be the same as the one set by the "container" class, adding this to the notebook would lead to the need to suppress a lot of the other properties introduced by that class for the left-right notebook view.
There was a problem hiding this comment.
I was thinking of putting the "80vh" value into a variable that is then used by container and notebook-container, just like we have done with $notebook-break etc. No need to add extra CSS classes then, we just want to move out the shared magic number into a shared variable
| padding: $spacer; | ||
| } | ||
|
|
||
| @media screen and (min-aspect-ratio: $notebook-break), (min-width: $notebook-break-width) { |
There was a problem hiding this comment.
You inverted the rule logic here. Before, we had the "non-selective" filters match the left-right layout, and then had a more selective ruleset (via media query) that changed it into top-down. Why is it necessary to invert this, what do we gain from that?
There was a problem hiding this comment.
My understanding is that there were two checks for both cases previously, and having both checks while introducing the fixed upper limit for the width introduced some weird behaviour when at the border of the checks. This is why I removed one of the checks and made the top-bottom view the default in the code, since the theoretical first instances of the screen at 1x1 px would the top-bottom view.
|
Placing still looks weird in some situations. Overlapping other content and I think the notebook should have the same width as the content if it's shown on top: Notebook too small (even close button doesn't fit): |
4 participants
fixes #2098