SCSS Review

[rickmanelius]

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.

[aManNamedJed]

• 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-colors map, as they made a couple of tweaks since the initial beta release. When we change the color of, say, primary within the map, then every instance of the -primary utility 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 tact

• When applying styles to the buttons, if we change primary and secondary within the $theme-colors map in our variables.scss, these button colors will automatically change. With that said, when applying styles to buttons in Bootstrap 4, we can leverage the button-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 rem in lieu of pt or px as 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 ability

[rfay]

@jdarrohn Could you do a PR with your proposals? Thanks!

[andrew-c-tran]

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 ?

[andrew-c-tran]

reviewed and merged #91 into master, thanks @jdarrohn !