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

Adjust trigger padding #102

Merged
merged 9 commits into from
Oct 26, 2024
Merged

Adjust trigger padding #102

merged 9 commits into from
Oct 26, 2024

Conversation

jimjam-slam
Copy link
Collaborator

Makes a small adjustment to padding for narrative blocks so that the padding is more reliable and gives the text more room to breathe by default

@jimjam-slam jimjam-slam changed the title Fix-adjust-trigger-padding Adjust trigger padding Oct 18, 2024
@jimjam-slam
Copy link
Collaborator Author

What do you think, @andrewpbray?

Before:

Screenshot of trigger before change with less padding

After:

Screenshot of trigger after change, with more padding

@@ -132,7 +140,7 @@
.narrative {
background-color: rgba(17, 17, 17, .85);
color: white;
padding: 5px;
padding: 5px; // TODO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO meaning consider revisiting if you want to tweak?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: don't get distracted halfway through writing a TODO 😂

I think I originally modified this property, realised this was inside the mobile layout, intended to decide whether this property was needed at all and then got distracted 😬 I think 0.75rem is a good bit of padding for all layouts though tbh, so I'm tempted to remove this altogether!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With padding: 5px on mobile:

image

With 0.75em (what I've set other layouts to):

image

0.3rem (vertical) and 0.5rem (horizontal) is a snugger but maybe balance that I think might better work for very small screens:

image

You could take a bit off the vertical for wider widths too! Maybe 0.7 0.75em?

@andrewpbray
Copy link
Contributor

While we're looking at that index closeread...

I used a very similar approach to line highlighting as the one used by revealjs. Essentially all non-highlighted lines in a block of lines with highlighting get hit with a .cr-hl-within class that changes opacity.

.cr-section .sticky-col .sticky-col-stack .cr-active.cr-hl-within .sourceCode span[id^=cb] {
  opacity: 0.3;
  transition: opacity linear 0.3s;
}

In a setting like this, where the whole cr-section div has a blue background color, some of that bleeds through since its opaque. Do you know of an easy way to fix this? Essentially I want a white mask on the code block, not really an opacity, or an opacity that sees past the blue background to the white beneath it...

@jimjam-slam
Copy link
Collaborator Author

jimjam-slam commented Oct 18, 2024

While we're looking at that index closeread...

I used a very similar approach to line highlighting as the one used by revealjs. Essentially all non-highlighted lines in a block of lines with highlighting get hit with a .cr-hl-within class that changes opacity.

.cr-section .sticky-col .sticky-col-stack .cr-active.cr-hl-within .sourceCode span[id^=cb] {
  opacity: 0.3;
  transition: opacity linear 0.3s;
}

In a setting like this, where the whole cr-section div has a blue background color, some of that bleeds through since its opaque. Do you know of an easy way to fix this? Essentially I want a white mask on the code block, not really an opacity, or an opacity that sees past the blue background to the white beneath it...

I think the blue is bleeding through because the .sourceCode background for the code block has a background-color with 65% opacity. When you set it to full opacity, the problem disappears:

image

Does that come from us, or from Quarto? That comes from Quarto (see the code blocks on this documentation page). We might just need to override it for this page...

@andrewpbray
Copy link
Contributor

Oh that's very interesting! Yes, a good thing to override manually for now but I wouldn't be surprised if other users run into this. If we see it crop up again, we can consider a more systematic fix.

Fundamentally, Quarto html styling probably (and sensibly) wasn't designed imagining that these elements were going to be put into a stack.

@andrewpbray
Copy link
Contributor

Hmm, I wonder if I'm missing something. When I go to pull, (and after i render to css), I get this look:

Screenshot 2024-10-18 at 6 27 09 PM

That looks a little tighter on the left than yours, no?

(this will be a big perk to the having the preview site!)

@andrewpbray
Copy link
Contributor

andrewpbray commented Oct 19, 2024

Hmm, I'm noticing some strange behavior on a narrow screen on the index at closeread.dev:

mobile-index-sm.mov

This was on Chrome on my laptop but this is the same thing I see on my (tiny) phone running Safari. When I check the ones in the Gallery, they don't seem to have this issue.

How does it work on your end? Maybe this is just an issue with .overlay-center... except on mobile don't we just switch to .overlay-center? so that wouldn't explain why the Gallery ones don't have this behavior on mobile.

@jimjam-slam
Copy link
Collaborator Author

jimjam-slam commented Oct 20, 2024

There are two things happening here, I think:

  1. Overflow (horizontal scroll bar) starts happening because the code block sticky has a minimum width, then
  2. The narrowest screen is just hitting the 575px width boundary for mobile sizing (which is overlay-center).

@jimjam-slam
Copy link
Collaborator Author

I've pushed some changes to improve overlay narrative positioning: instead of trying to use percentage margins, which are fiddly, it relies on justify-self to align the narrative column within the grid cell.

We could potentially still set a margin-left or margin-right to push the respective left and right overlay narrative content out from the edge. Or that could be a user YAML option 🙂

I've also set a max-width on the narrative content that is designed to line up seamlessly with the mobile breakpoint. That could also be a YAML option potentially!

To see the effect of it without the overflow getting in the way, add font-size: 70% to the div.code-with-filenameinside#cr-doc`:

Screenshot 2024-10-20 at 20 41 09 image image

To fix the code overflow on this page, I've:

  • added .scale-to-fill to the code block, allowing it to rescale as needed using transform: scale(), and
  • moved the font-size: 1.4em from the entire .cr-section to just the .narrative-col; .sticky-col is set to font-size: 0.6em.
    • Blocks that have .scale-to-fill still have layout space reserved for their "natural" width and height; the overflow happens when the window shrinks beyond those natural dimensions (even if .scale-to-fill has shrunk it visually). So generally we want to make those "natural" dimensions as small as possible by setting a low font size and having transform blow it back up. The trade-off we've observed is that Safari sometimes doesn't re-rasterise the text properly, making it blurry — but that doesn't seem to be happening in this case!
    • We should probably make sticky-natural-font-size a YAML option, since overflow issues will likely come up for users. We could also set a moderately default and try to get the best trade-off in terms of mobile support versus potentially pixelisation, but the natural width also depends on line length, so there's no one answer. For this code block, 0.6em is small enough to prevent overflow on widths down to 350px, which I think is pretty conservative in terms of device support.

@jimjam-slam
Copy link
Collaborator Author

You can briefly catch the re-rasterisation in Safari when changing highlights (not worried about it here since it's kicking in at the end of the transition, but you can at least see the un-re-rasterised state):

Screen.Recording.2024-10-20.at.21.16.15.mov

@andrewpbray
Copy link
Contributor

Did you add these changes to #107 instead of this branch? I still get the overflow issue when I try it locally on this branch.

@jimjam-slam
Copy link
Collaborator Author

Did you add these changes to #107 instead of this branch? I still get the overflow issue when I try it locally on this branch.

Oh jeez, I did 🤦🏻‍♂️ Lemme cherry pick them over to here!

@jimjam-slam
Copy link
Collaborator Author

jimjam-slam commented Oct 25, 2024

Should be good to test now, @andrewpbray!

(I tried to fix the conflict with the .css.map file when merging, but it looks like it's still in conflict with dev. I'd be tempted to accept one or the other and then just run sass again after the merge)

Copy link
Contributor

@andrewpbray andrewpbray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jimjam-slam jimjam-slam merged commit 8d2440b into dev Oct 26, 2024
1 check passed
@jimjam-slam jimjam-slam deleted the fix-adjust-trigger-padding branch October 26, 2024 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants