WIP - Student Notes UI - #6006
Conversation
@talbs I chose to use the existing layout already in LMS, even though it looks a little different from the wires. Let me know if the wires are the “new direction” for LMS and I’ll make the necessary changes. This commit also includes a bunch of icon font updates from another branch, so that I could use the new icon fonts in this work. It probably includes too many commits, but I wanted to make sure I got everything for LMS.
|
NOTE: This work is using some work that's currently underway in #6008 |
|
@clrux, as we chatted about here's a PR I'll be working through to review your FED work and also help out with visual styling on. We'll clean up your commits and its dependency on the Font Awesome update work as part of things as well. FYI, @explorerleslie. |
There was a problem hiding this comment.
I'm not sure this view needs to be included in this work. There's not a Studio portion of this UI work. Mind removing this?
There was a problem hiding this comment.
Please use and existing a typography font-weight placeholder.
|
@clrux, I've taken another spin through. Thanks for all of the extra attention to detail. One general thought - you may need to sync up with @frrrances on any RTL abstraction needed in this UI - the rules that come to mind are: |
One template that’s currently uses (_mixins) was updated with additional rules, so this shouldn’t affect anything that’s in production.
|
@frrrances @talbs This last commit should touch on all the PR comments above save one - the body class addition. Maybe we can review this bit next week? For now, I'll update my sandbox and ping Leslie for a review. |
f9798b4 to
0680643
Compare
b657710 to
3a79237
Compare
|
@talbs I've gone through the previous PR fixes and realized some of my work was lost over the break, so I re-did all of it today. Wasn't much, but I did change a few small things, so if you have time, it might be worth it to give it one more go at the PR reviews. @explorerleslie I'm going to set this up on a sandbox for you once I figure out what's going on with the provisioning (I can't access it for some reason). If you want to review this sooner (i.e., today or tomorrow), I'd be happy to show you locally. I especially want your thoughts on the blue. EDIT: I'm provisioning a new sandbox now. It should be up in an hour or so. The URL will be: http://clrux.m.sandbox.edx.org/ |
|
@clrux, given that the Font Awesome update work planned for here (https://github.com/edx/edx-platform/pull/6055) isn't in this current sprint, I'd decouple the commits for that work from here (just keep your commits for Notes UI + any Font Awesome syntax downgrade changes you need to make). |
|
@clrux, let us know when you have a sandbox - I'd like to review the styling/UI too. |
|
@talbs Sure thing. I think this work only has two icons, so I'll just reference the existing font icon set. As for the sandbox, it appears to be up! http://clrux.m.sandbox.edx.org/ |
|
@clrux I tried your sandbox, and I don't see the notes page (when logging in as staff@edx.org and accessing the demo course), and I also don't see the setting to turn on notes in Advanced Settings. Did you create this sandbox off of the work that @polesye did for the Notes page? A merged PR is #5968 and an in progress one is #6086. You may need to coordinate with @talbs and @polesye to get this set up. |
|
@clrux Is there a way to see the result for |
|
@polesye Not yet. I didn't branch my work off yours, so it's not there. I'm working on that now and will have something for you in a while. |
|
@clrux, is this branch and PR (that I opened for you a bit ago) still valid? Is there work here that hasn't been improved upon and already merged into the feature branch? |
|
@talbs Once this work was merged into |
4 participants
No description provided.