search

cfpb / generator-cf

Yeoman generator for Capital Framework
http://cfpb.github.io/capital-framework/getting-started/
Creative Commons Zero v1.0 Universal
9 stars 13 forks source link

Aligns brand palette with official palette #61

Closed anselmbradford closed 9 years ago

anselmbradford commented 9 years ago

Formats brand-palette.less to follow format and order shown in http://cfpb.github.io/design-manual/identity/color-principles.html#palette.

Additions

Scotchester commented 9 years ago

Moves @white to unofficial color group because it's not included as a swatch in the official palette.

Interesting. It was explicitly called out in the pre-Design Manual Brand Style Guide PDF. I guess we figured it wasn't really necessary.

Scotchester commented 9 years ago

Do you wanna go ahead and do the stuff we talked about in #60 here?

anselmbradford commented 9 years ago

Do you wanna go ahead and do the stuff we talked about in #60 here?

Let's keep that as a separate issue, kind of out of scope for here.

Scotchester commented 9 years ago

I lament the death of my beautiful table layout, but I get it :) I would just say that I don't think repeating those "x% tint" comments on every line is really necessary.

anselmbradford commented 9 years ago

@Scotchester I'm sorry, it was beautiful :(

anselmbradford commented 9 years ago

I don't think repeating those "x% tint" comments on every line is really necessary.

Agreed, the design manual has those %s. Fixed in https://github.com/cfpb/generator-cf/commit/92c6b3466ee33160875ff8357d1a83ad35ceb5ba

jimmynotjim commented 9 years ago

Aligned values, be still my :heart:

Oh and :+1: :smile:

Scotchester commented 9 years ago

:+1:

Scotchester commented 9 years ago

Don't forget to bump the version number and update the changelog.

anselmbradford commented 9 years ago

Don't forget to bump the version number and update the changelog.

Is this a notable change? I kinda thought changelog updates were for changes or fixes to the public facing API (i.e. the functionality), this is really a documentation update. I don't think you would update the changelog for changes to code comments, for instance. Sorry if I'm totally off, I'm still kinda getting comfortable with semver and wrapping my head around when it's necessary vs not.

Scotchester commented 9 years ago

You're absolutely right. It's just a reflexive instinct that I have for any change, at this point :) But no, it's not really necessary.

anselmbradford commented 9 years ago

Okay, commits squashed GTG

contolini commented 9 years ago

@Scotchester @anselmbradford I don't have a strong opinion about changelogs but any change to a codebase warrants a version bump. Semver doesn't say anything about changes being notable. This PR would be classified as a backwards-compatible bug fix and we'd update the patch number. This has already been squashed and merged so it's not a big deal but is something to be aware of going forward.

Why is it important? Static analysis! Someone or something out there could be relying on the code in this repo to be in a specific format. Us changing the format, even if we're not adding or removing functionality, might break their software. For example, this very repo uses regex to pick apart OSPT. If OSPT added a simple line break, it might break this generator. Don't get me wrong, relying on regex is a Bad IdeaTM, but it's good practice to be a pal and always bump. Changelogs are for humans. Versioning is for machines.

We do have the "notable" language at the top of our changelog. We can either remove the word "notable" and record all bumps in the changelog or leave it and only record important ones. Any preference?

anselmbradford commented 9 years ago

Thanks @contolini, I'm still fairly fresh with (maintaining) semver and I was mostly going off of http://keepachangelog.com, which talks about the changelog being curated.

I do have some questions about the argument around static analysis though. Static analysis should be running against a tagged release of the source tree, such as https://github.com/cfpb/generator-cf/tree/0.5.0. If another project is following the latest and greatest and there's a breaking change in the codebase, it shouldn't be a surprise. The onus is on a dependent project to follow the released codebase, not the untagged codebase.

In cfgov-refresh, it came up that we probably should be including all changes in an "unreleased" section at the top of the changelog with our PRs (in practice this hasn't been happening—but I'd like to get better with it), which could then be incorporated into a release when a commit was tagged. This keeps documentation on the changes in the changelog, but also prevents frivolous tagging of minor adjustments to the codebase.

On the other hand, if the idea is to tag every change, then we are essentially following the commit history and should look into automatic changelog generation instead.

Scotchester commented 9 years ago

I think I agree with @anselmbradford on dependent projects using a known tagged release, and with the idea of putting a change like this in the Unreleased section of the CHANGELOG. That way, it's not forgotten when the release is made.

It seems a bit much to release something that makes no functional change to the code, as it's intended to be used. Relatedly, when do you decide to release if you're making a number of changes in a short period of time? The static analysis argument doesn't sway me. Why does it matter if we bump it now, or along with the next batch of changes that actually do something?

contolini commented 9 years ago

I see where you guys are coming from. In the past I've always used automated tasks to handle things like changelogs (see grunt-cfpb-internal) because I'm lazy and don't value them a whole lot for simple things like websites. I don't have a strong opinion about doing an "unreleased" section so if you guys want to, I support it.

anselmbradford commented 9 years ago

@contolini grunt-cfpb-internal is good to know about! Looks cool. I'm all for automation too, it seems like it wouldn't be terribly much work to have grunt-cfpb-internal support an unreleased section as well.