10000 Include print styles in main stylesheet by deivid-rodriguez · Pull Request #6922 · activeadmin/activeadmin · GitHub
[go: up one dir, main page]
Skip to content

Include print styles in main stylesheet #6922

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 11, 2021
Merged

Include print styles in main stylesheet #6922

merged 1 commit into from
May 11, 2021

Conversation

deivid-rodriguez
Copy link
Member

No need to do this separately which makes an extra http request, plus this has the benefit of delaying print styles until needed. I’ve been doing this for a long time now and as its been well supported to include print styles using a media block. The HTML5 Boilerplate has stressed this as a good default approach for many years now. https://github.com/h5bp/html5-boilerplate/blob/master/src/css/main.css#L205-L209

Extracted from #3862.

Closes #6908.

@deivid-rodriguez deivid-rodriguez mentioned this pull request Apr 28, 2021
Closed
javierjulio
javierjulio reviewed May 7, 2021
View reviewed changes
Copy link
Member
@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

I think this will be worthwhile, thank you!

app/assets/stylesheets/active_admin/_base.scss Outdated
Comment on lines 8 to 43
@media screen {
@import "./header";
@import "./forms";
@import "./components/comments";
@import "./components/flash_messages";
@import "./components/date_picker";
@import "./components/tables";
@import "./components/batch_actions";
@import "./components/modal_dialog";
@import "./components/blank_slates";
@import "./components/breadcrumbs";
@import "./components/dropdown_menu";
@import "./components/buttons";
@import "./components/grid";
@import "./components/links";
@import "./components/pagination";
@import "./components/panels";
@import "./components/columns";
@import "./components/scopes";
@import "./components/status_tags";
@import "./components/table_tools";
@import "./components/index_list";
@import "./components/unsupported_browser";
@import "./components/tabs";
@import "./pages/logged_out";
@import "./structure/footer";
@import "./structure/main_structure";
@import "./structure/title_bar";

body {
@include sans-family;
line-height: 1.5;
font-size: 72%;
background: $body-background-color;
color: $text-color;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The admin CSS is already loaded as a screen media stylesheet so this isn't necessary. It would be best to remove the screen media wrapper part.

Suggested change
@media screen {
@import "./header";
@import "./forms";
@import "./components/comments";
@import "./components/flash_messages";
@import "./components/date_picker";
@import "./components/tables";
@import "./components/batch_actions";
@import "./components/modal_dialog";
@import "./components/blank_slates";
@import "./components/breadcrumbs";
@import "./components/dropdown_menu";
@import "./components/buttons";
@import "./components/grid";
@import "./components/links";
@import "./components/pagination";
@import "./components/panels";
@import "./components/columns";
@import "./components/scopes";
@import "./components/status_tags";
@import "./components/table_tools";
@import "./components/index_list";
@import "./components/unsupported_browser";
@import "./components/tabs";
@import "./pages/logged_out";
@import "./structure/footer";
@import "./structure/main_structure";
@import "./structure/title_bar";
body {
@include sans-family;
line-height: 1.5;
font-size: 72%;
background: $body-background-color;
color: $text-color;
}
}
@import "./header";
@import "./forms";
@import "./components/comments";
@import "./components/flash_messages";
@import "./components/date_picker";
@import "./components/tables";
@import "./components/batch_actions";
@import "./components/modal_dialog";
@import "./components/blank_slates";
@import "./components/breadcrumbs";
@import "./components/dropdown_menu";
@import "./components/buttons";
@import "./components/grid";
@import "./components/links";
@import "./components/pagination";
@import "./components/panels";
@import "./components/columns";
@import "./components/scopes";
@import "./components/status_tags";
@import "./components/table_tools";
@import "./components/index_list";
@import "./components/unsupported_browser";
@import "./components/tabs";
@import "./pages/logged_out";
@import "./structure/footer";
@import "./structure/main_structure";
@import "./structure/title_bar";
body {
@include sans-family;
line-height: 1.5;
font-size: 72%;
background: $body-background-color;
color: $text-color;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this is an unintentional mistake on my part, I'm loading the (now only) stylesheet with media="screen", but the stylesheet should be loaded for all media types, right?

If I understood your idea correctly, these blocks are correct because otherwise all the styles will be applied in the "print view" which is unintented (at least not how it works currently).

Copy link
Member

Choose a reason for hiding this comment

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

Yes that makes sense about the separate screen/print blocks because in #3862 the print styles were updated so many of the elements that were originally displayed in print mode weren't anymore. Thanks!

app/assets/stylesheets/active_admin/_base.scss
}
}

@media print {
Copy link
Member

Choose a reason for hiding this comment

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

I had this line defined in the _print.scss partial instead. I'm not sure but I figured that is required for it to work because I think Sass only allows @import "..."; at the outermost level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I moved it ouside, because since I was wrapping the rest of the styles under @media screen, moving it outside brought a nice simmetry in my opinion.

Not sure about Sass an not outermost imports, it seems to work just fine.

Copy link
Member

Choose a reason for hiding this comment

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

Ok great, thank you!

deivid-rodriguez
deivid-rodriguez commented May 7, 2021
View reviewed changes
Copy link
Member Author
@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks for having a look @javierjulio.

I replied to your comments, the PR as it was didn't work correctly. I'll push an update with a working version so you can re-review.

@deivid-rodriguez
Copy link
Member Author

This should be working now, but it seems that I need to fix some specs. Anyways, the idea of the outermost @media screen and @media print blocks is to make the styles mutually exclusive, which is how it was working up until now. I only left the normalize and typograhpy imports outside of the blocks because they were the only ones loaded by both stylesheets.

@javierjulio @deivid-rodriguez
Include print styles in main stylesheet
14b64cd 
No need to do this separately which makes an extra http request, plus
this has the benefit of delaying print styles until needed. I’ve been
doing this for a long time now and as its been well supported to
include print styles using a media block. The HTML5 Boilerplate has
stressed this as a good default approach for many years now.
https://github.com/h5bp/html5-boilerplate/blob/c9f88cc79d380150e4e2e39e3c2deccfe8aa3d8c/dist/css/style.css#L189-L193
@deivid-rodriguez
Copy link
Member Author

Alright, I spotted yet another error. I had changed the load order for the print media, so that a couple of SASS variables were being set after the typography import was using them, so they were not getting applied. I think the current version should be 100% equivalent to what we have now, and only provide the improvement of loading a single stylesheet.

@javierjulio
Copy link
Member

Yes that sounds right and makes sense. The two I had changed/removed were normalize and typography partials when working on the print portion. Thank you! I will approve and leave for you to merge when best. 🙌🏻

javierjulio
javierjulio approved these changes May 7, 2021
View reviewed changes
Copy link
Member
@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thank you! ❤️

@deivid-rodriguez deivid-rodriguez merged commit f083634 into master May 11, 2021
@deivid-rodriguez deivid-rodriguez deleted the media_print branch May 11, 2021 13:32
@baarkerlounger
Copy link

@deivid-rodriguez would it be possible to do another release to include this now that it's merged?

@deivid-rodriguez
Copy link
M 57AE ember Author

Sorry for the delay, I'll work on a release 👍.

@deivid-rodriguez deivid-rodriguez mentioned this pull request Mar 8, 2022
Merged
@Adeynack
Copy link
Adeynack commented Feb 24, 2023 •
edited
Loading

@deivid-rodriguez Was it released since? I can't find references to issue 6908 in any release note. I am upgrading an app from Rails 6 to Rails 7 and I get Sprockets::FileNotFound: couldn't find file 'active_admin/print.css'. Trying to understand the issue and came upon this PR. I am using the latest published Active Admin GEM (no version limit in my Gemfile).

EDIT: My deepest apologies. Just realized my project had a direct reference to that file in the manifest, and the file got renamed to _print.css since. Fixed... but still interested in knowing why I can't find a reference to that fix in the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierjulio javierjulio javierjulio approved these changes

Assignees

@deivid-rodriguez deivid-rodriguez

Labels
type enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue with Webpacker 6 (beta) and print.css
4 participants
@deivid-rodriguez @javierjulio @baarkerlounger @Adeynack
0