-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Conversation
Removes left padding as well as buttons and navigation for print.
Added _print.scss to styles directory, and added import for it in app.scss. Removed print styles from _temp_misc.scss.
Includes a definition of a new class ‘print-section’, which will only show on a printed report. In translations.js, added the string messages.for_authorized_persons with the message: “This report is for authorized persons only.” (Will require actual translation into additional languages, in this commit I just used Google Translate for placeholder translations into all the current language files). Added a header to the section template to read messages.for_authorized_persons text, using the ‘print-section’ class.
Thanks for the pr @dapierce ! We will take a look at this. @tangollama what do you think? @jglovier any issues/concerns about the css changes here? |
@dapierce can you please add an image of the final outcome here for quick reference? |
@jglovier here are a couple examples: |
There a couple things we could clean up later, but in general this looks great and is super helpful. 🤘 👍 ⚡ 🚢 |
|
||
.print-section { display: block; text-align: center; } | ||
|
||
.panel-footer, .btn, .nav, .view-current-title { display: none; } |
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.
Each selector should be on their own line. In fact I thought we had a test for that in the Stylelint. Wonder why it's not failing the build? cc @billybonks
Found some CSS syntax issues that need cleaned up before merging. Wonder why Stylelint isn't failling the build on these though? |
@jglovier Not sure why stylelint isn't failing the build here, but if I test this code change with the latest from master, it does throw an error:
@dapierce can you correct this issue? We should be able to accept the PR once this is set. |
@jkleinsc because he is using a colour that hasn't been set to a variable 😄 |
I will change to color variables in the print scss--been away for a bit, but I should be able to update the PR this week. |
@dapierce i love the fact that you added translations into every locale 👍 . |
I have updated my PR, but I'm not really sure why the tests are all failing on this one. EDIT: I found the part where it combs the stylesheet, I'll make another update to this! |
margin: 0 5%; | ||
font-size: 12pt; |
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.
Should use px
values here.
@dapierce btw for your convenience, if you are using Atom there is a handy linter package you can install that will show you SCSS linting failures in real time as you work, and what the fix is. More on that here. |
Changing to px units
Changed the method of positioning the printed page to fit scss lint.
I have made updates to the _print.scss that fixed the printed page margin and pass the lint test. I tried Atom and the scss lint, however I was unable to find the lint rules file (the readme github link doesn't work) and it seems by default the rules are different than our project's rules. |
@dapierce Looks good to me. I'm going to merge this PR in. |
@dapierce oooff. My bad. Those docs need updated. We're using Stylelint now, not SCSS Lint anymore. 😖 |
Fixes #476 Printing a patient record
This might not be exactly as envisioned, but I would appreciate any feedback. Thanks!
cc @HospitalRun/core-maintainers