Skip to content

Legacy dash cleanup - #6014

Merged
sarina merged 13 commits into
masterfrom
sarina/legacy-dash-cleanup
Dec 9, 2014
Merged

Legacy dash cleanup#6014
sarina merged 13 commits into
masterfrom
sarina/legacy-dash-cleanup

Conversation

@sarina

@sarina sarina commented Nov 20, 2014

Copy link
Copy Markdown
Contributor

This PR updates the legacy dashboard by removing all buttons and features that are present on the Instructor Dashboard. The advantage of this PR is making very clear upon inspection what few remaining features need to be ported to the Instructor Dashboard. As follow on pull requests, I would like to encourage those using the few remaining features to submit PRs porting the buttons to the new dashboard and deprecating the functions Legacy Dash in the same way done in this PR.

This also removes a lot of unstable and untested code, which is good for the health of our codebase.

I've added a deprecated class to the dash CSS so we can stylize messages as we see fit.

Screenshots of each Legacy Instructor Dashboard section are here: https://drive.google.com/a/edx.org/folderview?id=0B0jBaSRrrplqU3phdGo1dTZjbGM&usp=drive_web

@sarina
sarina force-pushed the sarina/legacy-dash-cleanup branch 2 times, most recently from ab9c37e to c65fe1a Compare November 21, 2014 15:19
@sarina

sarina commented Nov 21, 2014

Copy link
Copy Markdown
Contributor Author

This will need code review: @adampalay
UX review: @talbs
Product review: @explorerleslie

If we can get this roughly in shape, I will be sure to run it by people at MIT (Ike, Carson) to make sure I'm not removing something that shouldn't be removed.

@adampalay

Copy link
Copy Markdown
Contributor

👍 , thanks!

@sarina

sarina commented Nov 21, 2014

Copy link
Copy Markdown
Contributor Author

@adampalay was just curious if any of the buttons I left have an analogue in the new instructor dashboard. I didn't think they did - but it would be cool to remove as many as possible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can help make this a bit more explicit. Also, I'll change this to follow some of our SMACSS principles around stateful class names - https://smacss.com/book/state.

@talbs
talbs force-pushed the sarina/legacy-dash-cleanup branch from 4019179 to 8029b2c Compare November 21, 2014 16:53
@talbs

talbs commented Nov 21, 2014

Copy link
Copy Markdown
Contributor

@sarina, I took a quick spin through - this makes sense and I've added some general deprecation styling in to help support you. Looks good from a UX/FED PoV.

👍

@adampalay

Copy link
Copy Markdown
Contributor

@sarina we could also get rid of "Download CSV of all student grades for this course" in the "data download" tab

@sarina

sarina commented Nov 21, 2014

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @adampalay and the styling @talbs ! I've updated the screenshots in the Google drive folder to reflect the most recent version of this branch.

@adampalay

Copy link
Copy Markdown
Contributor

👍

@sarina

sarina commented Nov 24, 2014

Copy link
Copy Markdown
Contributor Author

Thanks Adam. Need to get @explorerleslie 's feedback and approval before merging

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging community manager review labels Dec 1, 2014
@sarina

sarina commented Dec 9, 2014

Copy link
Copy Markdown
Contributor Author

@mhoeber @catong - approved by Product. Could I get someone to quickly review the wording (see the screenshots in the description, or view at http://sarina.m.sandbox.edx.org/courses/edX/DemoX/Demo_Course/legacy_instructor_dash)

@sarina

sarina commented Dec 9, 2014

Copy link
Copy Markdown
Contributor Author

@explorerleslie - @shnayder took a look through this at last week's product triage, but he wanted to make sure you had a solid heads up. also need to get Doc to double check the wording.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like a duplicate of the line below ("To download student grades"). I would delete this one.

@sarina
sarina force-pushed the sarina/legacy-dash-cleanup branch from c9d06d2 to 08c3f40 Compare December 9, 2014 16:03
@sarina sarina removed community manager review product review PR requires product review before merging labels Dec 9, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For all the redirect messages:

  • delete "please" from the instructions
  • remove the single quotes around the section names
  • Instructor Dashboard should have initial caps.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd suggest combining redirect messages under each heading wherever possible so that there is only one redirect message under each heading, otherwise it's quite busy and might be confusing.
Minor rewording in this case:
"To view the Gradebook (only available for courses with a small number of enrolled students), visit the Student Admin section of the Instructor Dashboard."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also a thought about the positioning of the messages.
Not to make more work, but it might be less distracting if all messages were below any remaining buttons?
Users might come to a legacy dashboard page, and go to perform an action. If the button is there for their desired action they can click it. If not, any redirect messages are collected below all active buttons in each section. What do you think, @sarina, @explorerleslie @mhoeber?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Combine into one message (even though slightly different areas?)
"To manage beta tester roles and cohort groups, visit the Membership section of the Instructor Dashboard"

@catong

catong commented Dec 9, 2014

Copy link
Copy Markdown
Contributor

One additional suggestion although it adds work. In some sections where there are multiple redirect messages that cannot be combined because they redirect to different places, it might be better for users if all the redirect messages are moved below the active buttons. That way, if they are using the legacy dashboard to perform the actions, they can more easily see what actions are available, then any redirect messages can be seen below. What do you think @sarina @explorerleslie @mhoeber?
Otherwise, 👍 with suggested edits including combining messages where possible.

@sarina

sarina commented Dec 9, 2014

Copy link
Copy Markdown
Contributor Author

@catong I think moving the messages below the active buttons makes a lot of sense and a consistent UI. I've done that, and combined messages as you suggest.

@sarina
sarina force-pushed the sarina/legacy-dash-cleanup branch from 4ad8f54 to a1c8f8c Compare December 9, 2014 18:57
@sarina

sarina commented Dec 9, 2014

Copy link
Copy Markdown
Contributor Author

build passed at https://jenkins.testeng.edx.org/job/edx-platform-report/7774/ but had to squash my few fixup review comments.

@sarina

sarina commented Dec 9, 2014

Copy link
Copy Markdown
Contributor Author

Failing tests are flaky video tests, and the last build run on this exact branch (pre-commit-squashing, which had no conflicts) passed.

sarina added a commit that referenced this pull request Dec 9, 2014
@sarina
sarina merged commit 69fd063 into master Dec 9, 2014
@sarina
sarina deleted the sarina/legacy-dash-cleanup branch December 9, 2014 20:05
@explorerleslie

Copy link
Copy Markdown

@sarina yes, you get a solid 👍 from me. Thanks!

@sarina sarina added the open-source-contribution PR author is not from Axim or 2U label Apr 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engineering review open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants