-
Notifications
You must be signed in to change notification settings - Fork 54
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
#2858 Added feature for showing contributions hours view and PDF letter download #3837
Conversation
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.
The code looks good to me!
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.
This is super exciting! Folks are really going to like having this.
Here's the changes the letter needs:
-
Can we add an extra line/carriage return after the date and before the "To Whom it May Concern"?
-
Every place we say "From the Page" it should be "FromThePage" (my example letter was wrong on this).
-
Dates in the text of the letter need to be formally written out as well:
-
In the table, could we give the collection name more space and the page count less space? (Maybe right justify the page counts?)
-
Let's add another carriage return after "Regards,"
-
There should be a line break between "Sara Brumfield" and "Partner, Brumfield Labs".
-
In two places in the text, we break the words with a hyphen:
I'd really prefer that the word moves to the following line. (@benwbrum may have some ideas on that.)
I'm not entirely sure what to do about the line breaks with Let's right-justify the |
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.
Ok. 2 more small changes:
- Add a carriage return between the logo and the date.
- Make sure it's FromThePage, not FromthePage. (Branding!) It's wrong 2 places and correct 1 place.
Otherwise, looks good.
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.
Messages are missing:
letter (2).pdf
I would probably have added the messages to the dashboard messages file, to match the controller you're using. With your current setup, you can probably make this work by fully qualifying the message references, i.e. hours_letter.pdf.letter.certification_text
We use i18n-tasks to do a lot of this work; it makes it much easier. I'll send an email with some details about that library and our google translate key. (I probably wouldn't use it with this PR, because it requires starting over, but I would with all the ones in the future.)
No description provided.