Repository navigation
SCSS Review #89
Description
What happened (or feature request):
- @andrew-c-tran recently merged a PR that converted the existing CSS to SCSS.
- We've been attempting to get @andrew-c-tran from being isolated from the rest of the team as it pertains to code reviews, but these are areas where the rest of the team is less skilled within or rusty because most of the team is significantly more backend focused.
What you expected to happen:
- Full review of the SCSS including syntax, structure, organization, best practices, etc.
- Suggestions (when/where appropriate).
Please note:
- We're not looking for design level feedback at this time. There is another issue to capture that feedback.
- We're not looking for a PR at this time. I'd prefer to have a discussion to understand the level of effort and get feedback from @andrew-c-tran before proceeding.
- Please timebox this to no more than half a day. I'm assuming it can be done faster, but if you're finding that you need more time than that I'd rather discuss up front.
Activity
aManNamedJed commentedon Feb 26, 2018
Bootstrap 4 has essentially a color palette that we can leverage. You can check it out here. We may need to upgrade from 4.0.0 to 4.0.2 to be able to leverage the
$theme-colorsmap, as they made a couple of tweaks since the initial beta release. When we change the color of, say,primarywithin the map, then every instance of the-primaryutility classes, e.g.text-primary,bg-primary, etc. will all change accordingly. TWBS has also brought back the color variables, as well. We might as well use them if they are there.Let's try to avoid applying styles directly on top of the Bootstrap classes like
.card(Here) and.container(Here), really just for our sanity. I think it will make it easier for new people to come in and work on the UI without wondering why Bootstrap is misbehaving. If we want to change those styles, we can stick an additional class on them, e.g.drud-container, and still have the integrity of the Bootstrap classes in tactWhen applying styles to the buttons, if we change
primaryandsecondarywithin the$theme-colorsmap in ourvariables.scss, these button colors will automatically change. With that said, when applying styles to buttons in Bootstrap 4, we can leverage thebutton-variant()mixin found here. This way we can just grab the mixin in our branding and keep it all consistent across our digital touch points.Typically, I will user
remin lieu ofptorpxas the unit on font size, though the reason I do this is to not override the user's font size settings in their browser for accessibility, which may not be an issue given that we are in an Electron app and they might not have that abilityrfay commentedon Feb 26, 2018
@jdarrohn Could you do a PR with your proposals? Thanks!
andrew-c-tran commentedon Feb 26, 2018
I agree with your points and definitely thank you for pointing out the new $theme-colors maps that TWBS 4 beta utilize, they're pretty handy! As for r/em vs px, as this is a controlled electron app, we'll have control over it and I think keeping with px definitions is preferred for simplicity. As you've got the changes in an active PR, I think we can close this out.
Thoughts @rfay / @rickmanelius ?
Issue #89: Style/SCSS review (#91)
andrew-c-tran commentedon Feb 27, 2018
reviewed and merged #91 into master, thanks @jdarrohn !