Flowerhack/feature/bulkemailnewdash testingcoverage - #1215
Conversation
There was a problem hiding this comment.
For margin and padding please use the $baseline variable from _variables.scss
There was a problem hiding this comment.
You can also use $baseline/2, $baseline*2, etc.
|
Alright, this is an update on https://github.com/edx/edx-platform/pull/1180 and is ready for review. @sarina @adampalay @talbs @wedaly |
There was a problem hiding this comment.
why do you define the same method twice? Am I missing something?
|
I did a first pass. Tomorrow I'll check out your branch and run your code and probably make more comments once you've addressed mine. Other reviewers - we'd love to get this in before the next release so your immediate attention is appreciated. |
There was a problem hiding this comment.
Why define this in definitive pixels?
There was a problem hiding this comment.
I was trying to mimic the appearance of the old dashboard—is there a variable or something similar I should be using to set the width of this element (perhaps another "multiple of $baseline")?
There was a problem hiding this comment.
Ehhh. The original dashboard was far from well-architected from a FED-perspective. I took a look through really am at a loss when trying to compare UI, FED, and why you tried to sync this up.
Assuming that this beta will need to cleaned up from a design perspective down the road, I'm fine with this arbitrary, but working from your perspective, value. Fine with me for now.
|
Disable these just for the specific function/line
You should fix the unused argument and TODO errors. |
|
@wedaly Cool, thanks. Fixed up the quality issues as requested in the most recent update |
There was a problem hiding this comment.
Can you add in better text for the other two options? I'm thinking
staff -> "everyone who is staff or instructor on this course"
all -> "ALL (everyone who is enrolled in this course as student, staff, or instructor)"
Otherwise the popup message doesn't really emphasize very well the option (especially how scary the ALL is!!)
|
Whatever you did to fix the display on the beta dash, can you also apply to the legacy dash implementation? I'm not certain we'll be able to get the beta dash email implementation fully working & incorporating instructor task feedback before this needs to launch next week, so in case we can't yet turn on the beta dash implementation, it'd be good to have legacy looking right. At the moment, in my local dev the beta dash implementation looks fine but the legacy one looks wonky. legacy: |
|
👍, given the known FED and lack of design support issues. |
|
👍 Good to go once this gets a successful build on master. I'm going to merge #1296 as soon as the tests pass there to fix some broken acceptance tests on master, so you may need to rebase after that and trigger a manual build. |
set max enrollment for downloads to 200
Responses to Adam's comments; reset common.py, i18n compliance, deleted extraneous email.html file, fixed an HttpResponse, deleted unnecessary commented-out code, some small style tweaks
…_testingcoverage Flowerhack/feature/bulkemailnewdash testingcoverage
… removed tox-battery (#1215)
6 participants
Implements the "send email" section of the new instructor dashboard, with unit & acceptance testing.