-
-
Notifications
You must be signed in to change notification settings - Fork 578
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
feat: Adds print styles for Starlight docs pages #157
Conversation
🦋 Changeset detectedLatest commit: b7d6a24 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
2289ff4
to
f455c1c
Compare
57d6c69
to
819ea02
Compare
@delucis thoughts on whether this feature is worth merging or should I close it? I'm still a bit torn on the increase in bundled CSS versus pulling all print styles out to a global |
I think my preference is likely for pulling this out to a separate global file to avoid extra downloads. (We’d need a I am a bit torn because as you highlighted, that does come with extra maintenance complexity. Especially as this is exactly the kind of thing we’re likely to forget to check as we add stuff 😅 That said, most stuff is done pretty safely via (There’s a chance even doing that will fail the CI check here — it’s a bit of a rough tool at the moment, just globbing Let me know if you’re happy doing that or if you’d prefer me to tackle! |
@delucis Yep that's no problem, I'll refactor that this morning and pull the styles out into a global sheet 👍 |
@delucis done! Everything is moved out to the separate Looks like the HTML size-limit is failing by ~1kb, that should just be from the extra |
Best laid plans! It turns out there isn't currently a great way to get around the CSS being inlined there, I'm going to close this out for now as blocked. @delucis, @bluwy, and I were catching up on Discord and it sounds like this may be worthy of a feature in Vite to be able to |
A type check is failing while we wait on #2736, but I’m marking this ready for review nonetheless, as I think everything else is ready. Points to consider:
|
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.
Wow, such an amazing work and so hyped to finally see this PR being ready for review after so long! 🎉
Tested across multiple browsers, devices, etc. and most of the work is amazing! I also tested a bit on iOS, which seems fine, but it's super difficult to test without actually printing as the print preview is super small so I mostly focused on iPadOS which seems a more realistic use case to print from a tablet vs printing from a phone.
Here are a few questions and feedbacks:
-
Should a banner text color be
--sl-color-white
? -
Code block terminal frame looks weird imo with that empty window title bar, should we hide it or maybe make sure the 3 buttons are a bit visible (note that they are visible on Safari iOS and iPadOS only (not Safari desktop))?
-
Code block with a file name are missing a bottom bottom border below the tab bar (which is visible in terminal code blocks). I think it could be helpful if visible and also for consistency.
-
I would probably remove the tab top border in the code block with a file name?
-
Steps vertical line indicator is invisible.
-
Badges text is a bit difficult to read imo.
-
Should
blockquote
get anbreak-inside: avoid;
? -
Same question for lists?
-
Same question for tables?
-
Same question for preformatted text and figures?
-
Card grids may need a tweak regarding the breakpoint used as it seems a 1 column layout is always used?
-
The Copy button in code blocks should be hidden (if printing from a tablet, it would be visible and not useful).
Regarding some of the questions:
This PR uses print:* utility classes on some elements to control visibility on print. Are we happy with that? It’s the easiest way to do this I think (and allows users to even use these utilities in their custom components), but does mean print styles impact HTML a little.
I personally like the approach even more as a plugin author 👍
Do we think the styles are simple and flexible enough to be compatible with customizations to user sites?
I think they are, tested on a few different projects and I think the choice are flexible enough and thoughtfully done to not be too opinionated but just obvious enough when it comes to printing.
Thank you for the thorough testing and thoughtful comments! Lots of good stuff spotted and I think I’ve addressed almost all of this in the latest commits. Here are my notes one by one. Most of these I implemented and the couple I didn’t I left some comments.
Yes, probably and also the banner background reset — even though generally browsers do this automatically in my testing, setting it explicitly seems safest. Done in 1342f86.
Yeah, I also considered hiding it, but then realised that there can be a title on that bar potentially, so it’s not really safe to always hide it. Your comment made me think and I realised I could force the dots to appear in print styles by applying an inset box-shadow instead of a background. That way the SVG mask still works. Done in 1a36d9a.
Done in 20e47c9. I’m a little hesitant about these styles as they get into EC’s internal class names a bit. May be worth eventually chatting with @hippotastic about upstreaming some print styles to EC. Here’s an example of a print preview with these styles:
Fixed in bb2c313 using the same box-shadow technique as the terminal frame dots.
Agreed! Fixed in cd76d01 to use a more appropriate text color:
Expressive Code blocks do already have this set, as do the file tree, asides, and tab components. On the whole when I played with it, I felt like you don’t want to over do To answer the elements one by one:
I think this is expected because of the page width? Do you think it’s problematic to have these stack like on smaller viewports in print?
Ah, good point couldn’t notice this testing only on a laptop. Added a style that I assume should fix this in 43ed243, but couldn’t reproduce on my computer — will need to double check on a phone later. |
I'd gladly move any code snippet-related print styles into EC! Just let me know when you've reached a point where you are happy with the code block print styles, and don't worry about getting into EC's territory. I did not think about print styles yet and am thankful for the input. |
Thanks for all the great explanations. As most of the print stylesheets I've done in the past was me mostly applying suggestions from various sources and I never really print documents myself, my comment included a lot of questions and I appreciate the reasoning behind the decisions. Just tested all the points I reported on various devices and can confirm that your latest changes fixed all of them. I guess this also a topic where now that we have an initial version, we can iterate on it as we get more feedback from users that actually print documents.
Safari on iOS (not iPadOS) seems to be inconsistent in this regard so definitely worth setting it explicitly 👍
I'm not really sure what is expected. I noticed this when checking the print differences between https://deploy-preview-157--astro-starlight.netlify.app/resources/showcase/ which prints as I would personally expect vs https://deploy-preview-157--astro-starlight.netlify.app/resources/plugins/ so my initial thought was "Oh, we may need to combine some print media query with a size query to make it better" but without strong opinions on what is expected, I'm not really sure. |
Thanks for re-testing the changes! 💖
Definitely. I'm in a similar situation and although we've had some requests for print styles, I don't think they've included much detail so far. Will be happy to hear what people think.
Yeah, I'm not sure. I think it's more a coincidence that those two grid breakpoints fall either side of the page width. Given it's a bit of a pain to adjust media queries (usually needs a bunch of style duplication), I'd probably lean towards accepting it as it is, I think. |
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.
Forgot to approve with my last comment. Amazing work 👏
This PR is absolutely awesome! |
What kind of changes does this PR include?
Description