DO NOT MERGE (in #57): SASS diff from stanford themed branch for master #28
Conversation
ghost
assigned
lms/static/sass/course.scss
Outdated
There was a problem hiding this comment.
i think this needs to be commented out because the sass won't compile when it doesn't exist.
There was a problem hiding this comment.
Shouldn't this file be removed from source control entirely if it's being dynamically generated now?
There was a problem hiding this comment.
Yes, I asked @caesar2164 to throw this in just so that you guys can see what the site looks like when running, but in the templating PR I'll submit soon, this won't be needed. Definitely don't want to merge this file as is though, as @frrrances mentioned.
There was a problem hiding this comment.
This file is getting untracked and will be generated automatically once @natehardison's PR lands.
|
aside from what i mentioned above, and pending review by @ormsbee, this looks ready. and yes, we should definitely talk naming conventions, maybe later this week... or after 6/11. |
There was a problem hiding this comment.
Very minor, but I think you can just go with if env.get('THEME_NAME'): unless you need to check for None specifically.
There was a problem hiding this comment.
I'll make a note to change it in the templating PR.
|
@caesar2164 @frrrances @ormsbee thanks all for the work. One of the hard parts about this theming is that we have lots of interdependencies between the templating work I've been doing and the Sass work @caesar2164's done. From this PR, we do not need the changes to the Rakefile or the changes to the Thanks for the Saturday review. |
|
thanks @natehardison! i see how the changes in this PR make sense along with the other PR, though I'll need to check in with the rest of the team tomorrow morning to really get the full picture. thanks for all the hard work this weekend! |
lms/static/sass/base/_variables.scss
Outdated
There was a problem hiding this comment.
One thing we've tried to do recently with our Sass is to leverage Sass' extend functionality for defining static CSS rules we want to use over and over as an archetype. We tend to use mixins for anything we're passing arguements/variables into. See https://github.com/edx/edx-platform/blob/master/common/static/sass/_mixins.scss for a good example of what we're doing.
@caesar2164, would you mind making these extend definitions rather than mixins (it'll require you to also change your reference syntax elsewhere to @extend from @include)?
There was a problem hiding this comment.
@talbs - I think these do need to be mixins though, as these are theme specific and not class specific...
There was a problem hiding this comment.
@caesar2164 I don't agree with that logic.
Mixins and extends shouldn't be chosen for the purpose of differentiating whether something is related to a theme or a general re-usable styling element (this is what I'm assuming you're referring to as a "class"). That differentiation should happen within our other Sass/CSS practices - such as breaking things up into a core set of Sass rules/styles and overriding/extending with theme-based Sass after (what we're all doing in general with this theme add-on).
Here's a simple example of how extends and mixins are used, alongside how they render - https://coderwall.com/p/7p7w2a. Also, here's another rundown on how they're different - http://blog.kiskolabs.com/post/5445752361/extend-your-sass. Using extends for static shared css rules has a few benefits:
- Bloat - less bloat and redundancy from rendered rule to rule
- Code Clarity - differentiation to front end developers on when a value/argument needs to be passed in for proper use (extends don't take arguments, thus in an app/site that leverages extends, it would be clearer when arguments/settings, due to the @include(arguments) syntax are needed when using a mixin vs. a site/app that used mixins for everything.)
- Definition/Use in Context - extends can be defined in the appropriate context of our Sass architecture - we have a lot of global/utility classes/extends defined in the example I provided, but I have also started creating classes to extend in other places, namely in our typography settings (https://github.com/edx/edx-platform/blob/master/cms/static/sass/elements/_typography.scss) for Studio. This helps in managing base rules by grouping them by purpose.
If this will cause you headaches now in getting things going for 6/11, it can wait until a follow-up with theming is done. Its important we are all on the same page as a team on how to create re-usable and foundational styling rules longer-term
I'm happy to talk this over more on HipChat or a video chat too.
There was a problem hiding this comment.
After talking it over with @caesar2164 and @natehardison, this mixin is needed due to the limitations of the current theming implementation. @caesar2164's recommendation to move it to _shame.scss makes sense and we can revisit how to implement a theme's variables and overrides in the larger theming project.
|
Given that @caesar2164 is making a few tweaks (font-weight, moving mixins to _shame.scss) per today's comments/discussions, this set of tweaks is fine to be merged from my perspective. 👍 |
|
@talbs, @natehardison, @frrrances, @ormsbee - As the courses.scss is getting untracked in the templating PR, should I leave that file as is, change it, or what? Once that is dealt with, is this good to merge? |
|
👍 from me too. |
…esolve a Sass rendering issue
|
Things look good on this branch, but one issue that both @frrrances and I ran into when pulling form a local copy was a lot of local file merge conflicts. I'm not sure what is causing this, but we both reproduced it after previously checking out this branch and pulling from origin to it. I was able to do it a second time after deleting this branch locally and checking it out again. I'm not sure if this will carry over to master once this is merged in, but we don't want others running into this. Are you folks seeing this as well? |
|
I rebased this branch off of master a bunch of times and then had to force push...could that be what you're seeing? @natehardison agrees! |
|
Okay, makes sense. In you and @natehardison, I trust. 👍 |
|
👍 |
|
Closing in favor of pull request #57 |
* fix-reset-page: [#118698597] Add the navigation menu [#118698597] change reset detail page text and title [#118698597] Change email label [#118698597] remove irrelevant navigation from menus [#118698597] use context variable [#118698597] update reset details page [#118698597] Update reset details page Change route in url file
Syncing changes from oxa/master.fic to oxa/devfic: CDN Endpoint & MatPlotLib Fixes
Add extra fields for order detail API
[TNL-7072] When a sequence navigation would overflow, convert it to a dropdown.
@frrrances & @ormsbee - here are the straggling SASS changes to get the Stanford theming branch into master.
I replaced a few variables with general ones. (btw, going forward we all need to talk about variable naming and stuff...)
I also discovered how lovely SASS mixins truly are, and I think we should use them everywhere to enable nitty gritty theming...