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

Document the CSS #1871

Merged
merged 22 commits into from May 15, 2018

Conversation

Projects
None yet
3 participants
@hbillings
Member

hbillings commented May 8, 2018

Closes #1840, #1878 and #1891.

  • Add a section to /docs/frontend.md describing some development practices for CSS/Sass.
  • Document order in main.scss.
  • Document why and where the scattered bits of CSS not in frontend/source/scss are.
  • Comb through components and document anything funky (especially anything that Previous Heather noted was funky in a comment).

@hbillings hbillings requested review from toolness and ericronne May 10, 2018

@hbillings hbillings changed the title from [WIP] Document the CSS to Document the CSS May 10, 2018

hbillings added some commits May 10, 2018

@toolness

Nice, this looks great and will be really helpful! Thanks!

The only blocker is just the reference links vs. inline links thing in frontend.md. Once those are fixed up I think this is good to merge!

@@ -8,7 +8,7 @@
{% with total=rows|length %}
<div class="message message-error no-bg" role="alert">
<div class="usa-alert usa-alert-error alert__no-bg" role="alert">

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

For some reason I read alert__no-bg as "alert, no biggie."

This comment has been minimized.

@hbillings

hbillings May 14, 2018

Member

😂 alert__no-biggie would be a fantastic alias for alert__warning

@@ -4,7 +4,7 @@ In general, front end code is in the [frontend](../frontend/) directory.
This guide is about the nuts-and-bolts of developing front end code; for
details on how to use or style individual components, see the
[style guide][].
[style guide][https://calc-dev.app.cloud.gov/styleguide/].

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

Oh, this is actually defined further down in the file... it's actually a reference-style link, which I often like better when I know that folks will be reading the original Markdown text, because it makes the original easier to read (b/c the reader doesn't have to find the end of the URL to keep reading the original text).

Can you remove the URL you added here? Thanks!

This comment has been minimized.

@hbillings

hbillings May 14, 2018

Member

Oooh, those are new to me! I thought they were incomplete. I'll pull all those that I added out!

This project generally follows a modified (BEM)[https://frontend.18f.gov/css/naming/#bem] (Block, Element, Modifier)
naming scheme. This prevents namespace collisions and alleviates the need for too much nested Sass.
The `sass` files follow a few conventions:

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

Should this be Sass instead of sass, since you were using Sass earlier? Not a big deal either way, just wanted to point it out. Or I guess you could say "The files in the sass directory" if you're referring to those? Eh, I'm probably overthinking this.

This comment has been minimized.

@hbillings

hbillings May 14, 2018

Member

I think I was trying to call out the file extension, but that's scss so that doesn't even make sense. I'll make this more standard one way or another.

- Partials (also called includes or imports) are files that will get compiled into the main CSS file. These are prefixed
with an underscore (such as `components/_footer.scss`).
A more detailed explanation of how to use specific components can be found in the CALC [style guide][https://calc-dev.app.cloud.gov/styleguide/].

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

This is actually another advantage of reference links--you don't have to put the URL in there, as the Markdown parser will find the reference and hyperlink everything that uses it to the URL.

Also note that reference links use square brackets rather than parentheses (the other links are called inline links b/c the URL goes "inline" w/ the rest of the link text).

This comment has been minimized.

@hbillings

hbillings May 14, 2018

Member

ahhhh I was so confused about the square brackets and parentheses being mixed up. Thanks, I'll fix this!

### Data explorer
The data explorer is a [React][]-based app that uses [Redux][] for data flow and state management. It's located in[frontend/source/js/data-explorer](../frontend/source/js/data-explorer/).
The data explorer is a [React][https://reactjs.org/]-based app that uses [Redux][https://redux.js.org/] for data flow and state management. It's located in[frontend/source/js/data-explorer](../frontend/source/js/data-explorer/).

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

React and Redux are reference links, so no need to fill in the URLs here.

Also there's a missing space between in and the link at the last sentence.

Globally, we use [yarn](https://yarnpkg.com/en/) to manage our node dependencies and run node tasks.
`yarn`, in addition to being faster than using `npm install`, has the
benefit of locking dependency versions via a `yarn.lock` file.
Read the [yarn workflow docs](https://yarnpkg.com/en/docs/yarn-workflow) if you are not familiar with how to use it.

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

Yarn and yarn workflow docs are already reference-style links with their URLs at the bottom of this file, so no need to make them inline links... Although I guess if you prefer inline, please remove the reference links at the bottom of the file.

@@ -11,6 +11,7 @@ th.action-checkbox-column {
}
}
// same as .sidebar in overrides.scss

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

Hmm what does this mean? Is it the same exact CSS? If so, could we use a mixin or something to reduce the repetition and the need for the comment? (My main concern is that .sidebar in overrides.scss could get moved/changed/etc and this documentation could no longer be accurate.)

This comment has been minimized.

@hbillings

hbillings May 14, 2018

Member

A mixin's not a bad idea, though I'm not sure where to define it. These (overrides.scss and changelists.scss) are both Django override files, and we're styling two disparately named things to look the same. I'm inclined to let this be for right now, because I'm really hoping to do some more work on the admin interface (including maybe updating it so that these aren't divergent any more).

This comment has been minimized.

@hbillings

hbillings May 14, 2018

Member

Filed #1899 to look into this.

// Django's templating system, so they don't follow our BEM methodology and
// sometimes we hae to target things in clumsy ways.
// There are a number of !important declarations used when trying to override
// Django styles that are already at the highest possible level of specificity.

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

Nice explanation!

// This is largely a reimplementation of the US Web Design System's grid.
// At the time this was created, the WDS was incompatible with this project,
// having an old version of the Skeleton CSS framework in place. The naming
// conventions come from swapping that project out but retaining old code.

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

Noice explanation!

@@ -1,6 +1,8 @@
@import 'variables.scss';
// legacy sr-only class
// This class is applied to things that should ONLY be rendered to screen readers,
// such as descriptions of form fields that would be redundant for sighted users.

This comment has been minimized.

@toolness

toolness May 11, 2018

Contributor

Oh! USWDS actually has a .usa-sr-only that we could be using instead of this... should we file an issue to migrate to that eventually? I think we originally defined our own b/c of how we weren't originally using USWDS or whatever.

This comment has been minimized.

@hbillings

hbillings May 14, 2018

Member

Oh excellent! For some reason I couldn't find this last time I went poking around in the WDS and thought they'd gotten rid of it.

@ericronne

Much of your impressively extensive work here is beyond my ken, but i did learn something by reading it! So it's good documentation for this user, for one.

A color variable or two will need revision, but i will deal with that post-merge (#1900).

hbillings added some commits May 14, 2018

@toolness

toolness approved these changes May 15, 2018 edited

Nice this looks good to me, just the one comment!

@@ -79,11 +79,11 @@ available at `calc.gsa.gov/admin/`).
### Data explorer
The data explorer is a [React][https://reactjs.org/]-based app that uses [Redux][https://redux.js.org/] for data flow and state management. It's located in[frontend/source/js/data-explorer](../frontend/source/js/data-explorer/).
The data explorer is a [React][]-based app that uses [Redux][] for data flow and state management. It's located in[frontend/source/js/data-explorer](../frontend/source/js/data-explorer/).

This comment has been minimized.

@toolness

toolness May 15, 2018

Contributor

There's still no space between in and the link here, not that big a deal but would be nice to fix...

This comment has been minimized.

@hbillings

hbillings May 15, 2018

Member

oh whoops! thanks!

@hbillings hbillings merged commit 440ae12 into develop May 15, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/total-coverage 92% (0.0% change)
Details

@hbillings hbillings deleted the 1840-css-documentation branch May 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment