-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Studio: Course Outline Static Rendering/Styling #4419
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
Conversation
|
@andy-armstrong and @frrrances, this is ready for a UI review. I'd love your expertise and thoughts on the following:
I've add most UI elements back into the static template (seen locally at http://localhost:8001/template/ux/reference/outline.html), but here are a few things that still need to be added by the team in the near future:
FYI, @cahrens and @explorerleslie. |
|
This is looking gorgeous. I'm so psyched I'm going to start wiring it up right now... :-) |
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.
I thought we were going with class names like button, button-toggle, button-toggle-expand-collapse? I know that @frrrances is switching the unit page from add-button to button-add.
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.
Good to know - I'll sync up these and double check other button UI. Thx!
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 has been synced up.
|
@andy-armstrong, can you tell me what element those styles are from in your inspector? I can proof things once I have that info, |
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.
Using list-sections here is a little tricky for me. I don't know what the potential children are, I don't want to enforce a hierarchy, and even if I did I don't know how to pluralize the name! I could hack this for the key classes, but what will happen if we get a split_test included? I won't know whether the children are subsections, units or some kind of components.
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.
When we reviewed this in person last week, this semantic class naming around lists seemed okay.
If its going to be troublesome, we can definitely figure out a more flexible naming convention, but I want to avoid going back to the "list-sortable" original classes. I've looked through my Sass/CSS and my references to the list elements that contain outline elements are minimal, but I do need some sort of differentiator for subsection and unit lists (to add vertical spacing between them and their parents).
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.
I don't remember everything we discussed in the semantic review, but this strikes me as not accurate given that we don't know the child types. If you need the class, I'd prefer it if we could name it for the parent instead of for the child, e.g. something like 'list-course-children', list-section-children.
Having said this, for now I've put in the special case logic for the three cases and it doesn't seem so bad. I will be interested to see what happens with split tests. I guess I'll treat them as subsections.
|
Here's a child branch that starts to pull things together. It is still in very rough shape: |
|
@talbs Here's a case where my |
|
@talbs Hmmm, I see now that there's an explicit style on my buttons, so maybe this is my mistake where I'm manually showing and hiding the buttons. I'll investigate when I get into the office. |
|
Btw, here's how it looks right now. We definitely need to sync up on the new pattern for inline editable fields because I don't want to change the markup there as we agreed it with @frrrances and @dan-f, so that's why the titles have slightly odd stylings. |
|
@andy-armstrong, syncing up with @frrrances is on my list. Hopefully then we can iron out markup/semantics. Let me know once the liquid hot magma cools on your wiring branch - I can sync any lighter styling issues then. |
cms/static/sass/_variables.scss
Outdated
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.
What makes this color alt?
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.
Ack! I should remove this and the other alts (they are the RGB values you and @andywaldrop used in your wireframes+comps). I've moved to using Studio standard colors that you're using on the unit/container views for publishing status.
|
@frrrances, I think I've addressed all of your feedback. Let me know if there are any more concerns from a UI/visual/FED point of view and thanks for the support! |
|
Looks good! Thanks for addressing my comments. Not sure if you need tests to pass, but I'm 👍 once you check on that. |
|
Thanks! @andy-armstrong, I've rebased against bulk-publishing. Seeing as I haven't touched any existing/live views (my styling and markup work were done on static templates) are we worried about tests passing on this branch? If so, can you help resolve the failing tests? |
* static markup for all section states in outline UI * static markup for all subsection states in outline UI * static markup for all unit states in outline UI
* adding in outline-related color values * styling for publishing states * light styling abstraction for specific outline and unit/container contexts * revising add item to outline button styles * adding a few bells and whistles (hover states and error pulsing)
|
@andy-armstrong, I'm hands off now with all static styling. You should have what you need and I can tweak more of the interactive styling finishes once we've merged this and I'm on your branch. A few heads ups:
|
|
@andy-armstrong, tests continue to fail on this branch, but my work is not touching any "live" views. I'm only adjusting style and creating the static templates we've been working on. Would you mind taking a look at the tests and giving me the skinny on whether or not we should be worried about them failing? |
* sync up stateful class names * show status border/visual for subsections in collapsed and expanded states * hide new-centric actions and messages on collapsed items * show/hide content elements for outline containers based on collapsed states * button-new semantic changes and icon-padding visual tweaks * replaced draft-like icon to something not used for actions * refactored styling and markup for collapse/expand
|
@talbs All the failures are in the acceptance tests (both Lettuce and Bok Choy) because the CSS doesn't match the code anymore. In particular, it looks like the toggling behavior is causing most of the trouble. I think it probably makes the most sense to get everything running on my branch, and then we'll create a new branch where we cherry-pick your commits and mine together. We won't have to squash them but at least the single merge won't break the build. @cahrens Does this make sense to you too? |
|
@andy-armstrong, that method of cherry-picking sounds good to me. I'll rely on you and @cahrens to let me know when the timing is right. I'm hands-off on this branch now. |
|
Closing this in favor of https://github.com/edx/edx-platform/pull/4549. |



This work provides some rendering support of the bulk publishing-related course outline changes.