Instructors can view previously sent email content #4451
Conversation
|
@jinpa @jrbl @sarina |
|
These screenshots LGTM! On Wed, Jul 16, 2014 at 11:53 AM, Nicholas Dupoux notifications@github.com
|
|
Hi @njdup - right now we're sprinting towards a few commitments that are very time-sensitive. I hope to take a first pass at this PR over the coming weekend (perhaps evenings if I can manage it!) because I really do want to get this feature in. Once I take a first pass, I'll be able to have a good estimate of what type of UX and doc support we'll need to get this merged in. Thanks for your patience! |
|
@sarina Thanks for the heads up and its no problem at all, take all the time you need! |
|
Has @caesar2164 looked it over to confirm it meets our usual standards? |
lms/static/js/toggle_login_modal.js
Outdated
There was a problem hiding this comment.
you probably should remove this line… :P
|
You can catch a lot of nitpicky cosmetic things by running: |
|
The cells in your Content History Table screenshot look really big. Couldn't those be made less tall? |
|
I like the email content modal. |
There was a problem hiding this comment.
You can cut this line altogether
|
@njdup - remember to add padding to cells, make email title more visible as links, and move button in modal to top right. |
There was a problem hiding this comment.
I'm not wild about mixing strings and numbers or hardcoding strings. Would it work to set this to None if it breaks, and have whatever consumes email_feature_dict know that for number_sent None means unknown? It sort of looks like you're already doing this in the coffeescript anyway...
There was a problem hiding this comment.
Oh great point joe, setting it to None works since I check for it in my coffeescript table rendering anyway :)
|
Added styling feedback from @caesar2164. I'll work on incorporating your feedback next @jrbl. Thanks guys :) |
|
Those screenshots are pretty pretty. On Wed, Jul 16, 2014 at 5:16 PM, Nicholas Dupoux notifications@github.com
|
There was a problem hiding this comment.
"Click on an email's subject line to see the message content."
There was a problem hiding this comment.
Actually let's do this it looks great: <p><em>"Click on an email's subject line to see the message content."</em></p> <br />
At this point it would be easier, and better, to just do this in the template. Add a new
class region with the appropriate stylings and then populate it with text.
|
@jinpa sure - actually I think if my num. 2 recommendation is implemented, it obviates the confusing workflow that we were seeing. So yeah, let's not clear on send - but do clear before loading in something from history. |
There was a problem hiding this comment.
UX would like a different layout here. I mocked out what they want (easier than drawing), but implementing it will require a bit of changes to your Coffeescript since there are new divs with new names. Note too the addition of new text and a change to the text value of the button.
|
Overall UX was primarily concerned with the page layout as well as your Sass scoping rules and modal overrides (use the default inherited style wherever possible - may require changing your variable names so the right rules are picked up). Otherwise they were very impressed - they like the table and the modal, very clear and easy to use. |
|
Also, you're going to need to rebase your PR before we can merge it, regrettably :( |
|
@sarina that sounds sane to me. On Wed, Jul 30, 2014 at 1:36 PM, Sarina Canelake notifications@github.com
|
|
I think it makes sense to copy the subject as well. Although I'm interested
|
|
Just passed it by her and she agrees. Thanks :) |
|
@sarina per the UI feedback from you and the design time, I've made the following changes:
I've also rebased and squashed. I think that should be everything, let me know if I missed anything :) |
There was a problem hiding this comment.
@njdup, could you provide a screenshot of where this will appear? (Generally I'd suggest following the "problem first, solution second" style that (mostly) is used on the dashboard, but would like to see the context.)
There was a problem hiding this comment.
"To see the content of all previously sent emails, click this button:"
(@sarina, I validated the problem first, action second" structure with @frrrances (UX).)
|
@lamagnifica I'm trying to spin up a sandbox w/ this on it so you can test and take screenshots that you need. However, it's not working - looks like another commit (not this one) has broken Javascript across a lot of pages. Until we can fix that issue we won't be able to merge this anyway. So let's sit tight. |
|
@lamagnifica I've got this running now on a sandbox. Please visit http://njdup.m.sandbox.edx.org/ and test it out by visiting the Instructor Dashboard > Email tab ❗ 😄 |
|
Awesome, just let me know what decisions are made and I'm ready to revise as needed :) |
|
@njdup please make the last two changes suggested by @lamagnifica and then we'll click the button! |
Previously on the send email page of the instructor dashboard, instructors could only view task information about emails they've sent for their course in the past. In addition to this, I've now added the ability to see the content of all previously sent emails. A "Sent Email History" button has been added to the page. When clicked, a table displaying the subject line, number of emails sent, and date/time of submission for each previously sent email is created. An instructor can then click on any subject line to see the content of that email, displayed in a modal window that appears on the page. The window is also equipped with a "copy email to editor" button, which copies the emails contents to the tinyMCE editor, so that an instructor can easily resend an email that they've sent in the past.
|
@lamagnifica Thanks for taking a look at this and for your suggestions! @sarina I've made the final changes and rebased, everything should be up to date and ready to go :) |
|
@njdup thanks so much for this contribution. I'm so happy to have it and it's been a real pleasure working with you! I'll merge it once a build's been run on it! |
|
Thanks @sarina, it's been a pleasure working with you too :) |
|
👍 The task to document this feature will be in the next documentation team sprint (starts tomorrow). |
…history Instructors can view previously sent email content
6 participants
Previously on the send email page of the instructor dashboard, instructors could only view task information about emails they've sent for their course in the past.
In addition to this, I've now added the ability to see the content of all previously sent emails. A "show email content history" button has been added to the page. When clicked, a table displaying the subject line, number of emails sent, and date/time of submission for each previously sent email is created. An instructor can then click on any subject line to see the content of that email, displayed in a modal window that appears on the page. The window is also equipped with a "copy email body to editor" button, which copies the emails contents to the tinyMCE editor, so that an instructor can easily resend an email that they've sent in the past.
This PR addresses some aspects of this ticket: https://openedx.atlassian.net/browse/LMS-10345