Version 2

[chris-pearce]

Non-backwards compatible

@kllevin @chriswrightdesign @stowball I know this is massive but I've tried to keep the commits as concise as possible so probably better to go through each commit rather than reviewing everything. The issues above marked with TODO I still need to do.

[chris-pearce]

@kllevin updates after your code review: bb4f03c.

[chris-pearce]

#70 done, see commit.

[chris-pearce]

#41 done, see commit.

[chris-pearce]

#67 done, see commits.

[chris-pearce]

Remaining is #65 and #73 which are documentation changes so it's pretty much done from a code POV 👍

[chris-pearce]

@chriswrightdesign @stowball could I get your eyes on this as I'd rather not merge until you've looked at it? I want to merge it before next Mon.

[chris-pearce]

@kllevin could you take a look at the last few commits before this gets merged? Thanks.

[chris-pearce]

@stowball FYI minor versions do not break backwards compatibility only major:

1. MAJOR version when you make incompatible API changes,
2. MINOR version when you add functionality in a backwards-compatible manner, and
3. PATCH version when you make backwards-compatible bug fixes.

http://semver.org/

[stowball]

Ah cool. Good to know

[kllevin]

LGTM, other than the small comment on button groups.

[stowball]

The style.css file doesn't look like it's up to date. If I do a sass watch, the file has a few modifications, Personally, I wouldn't include this generated file in the repo

[stowball]

I think we should resolve #68 so that ..c-button-faux-link text-decoration is correctly set before v2

[stowball]

This is line is incorrectly indented https://github.com/chris-pearce/scally/blob/v2/core/base/_links.scss#L20

[stowball]

Did you mean to remove the display: table and display: inherit utilities in this 3569a05#diff-f98ee8d4f77940181d5ccfbc218fe1abL83? They might come in handy

[stowball]

In _u-spacing.scss, change https://github.com/chris-pearce/scally/blob/v2/utilities/_u-spacing.scss#L1154 to:

@each $size, $value in $u-spacing-sizes {

so that there's one less thing to update should we add another spacing size such as mega

It would be great to make all of the classes dynamic and automatic, but that seems like one hell of a job 😢

[stowball]

Incorrect indentation (tabs & doubled) here https://github.com/chris-pearce/scally/blob/v2/objects/_o-flexible-embed.scss#L139

Actually, there are 14 tabs throughout Scally that need changing.

[stowball]

Shouldn't https://github.com/chris-pearce/scally/blob/v2/core/base/_links.scss & https://github.com/chris-pearce/scally/blob/v2/objects/_o-link-complex.scss $link-color(-hover) variables be $color-link(-hover) to be consistent with other vars?

[stowball]

These dark and light overlay modifiers don't look right https://github.com/chris-pearce/scally/blob/v2/objects/_o-overlay.scss#L142. Classes on silent placeholders?

[stowball]

Apart from these 8 comments and 2 PRs, v2 looks awesome. Good work @chris-pearce 👍

[chris-pearce]

@stowball thanks for the feedback, some replies:

---

The style.css file doesn't look like it's up to date. If I do a sass watch, the file has a few modifications, Personally, I wouldn't include this generated file in the repo

Yeah I'm going to look at removing this from the repo as it's shouldn't be there, it was included to satisfy the main property for the package .json files, Bower's version. It should be style.scss, which I tried when first setting them up but was getting a funny error with Bower. Anyway will revisit.

---

I think we should resolve #68 so that ..c-button-faux-link text-decoration is correctly set before v2

I was going to do this as part of the v2 release, both #68 and #97.

---

This is line is incorrectly indented https://github.com/chris-pearce/scally/blob/v2/core/base/_links.scss#L20

I think you fixed that in #89? I'll be doing a check of all files and finding a Sublime Text add-on to stop this happening furthermore.

---

Did you mean to remove the display: table and display: inherit utilities in this 3569a05#diff-f98ee8d4f77940181d5ccfbc218fe1abL83? They might come in handy

Yes that was intentional as I'd never used them in all the time they'd been there and it was quite some time. I'm happy to go with what's in the Display utility now, let's see how they go and see if they're many requests to add them back.

---

In _u-spacing.scss, change https://github.com/chris-pearce/scally/blob/v2/utilities/_u-spacing.scss#L1154 to:

@each $size, $value in $u-spacing-sizes {

so that there's one less thing to update should we add another spacing size such as mega

It would be great to make all of the classes dynamic and automatic, but that seems like one hell of a job 😢

Cheers for that and yeah massive job :)

---

Incorrect indentation (tabs & doubled) here https://github.com/chris-pearce/scally/blob/v2/objects/_o-flexible-embed.scss#L139

Actually, there are 14 tabs throughout Scally that need changing.

You sorted 👍.

---

Shouldn't https://github.com/chris-pearce/scally/blob/v2/core/base/_links.scss & https://github.com/chris-pearce/scally/blob/v2/objects/_o-link-complex.scss $link-color(-hover) variables be $color-link(-hover) to be consistent with other vars?

These setting names are correct. All settings start with the thing they're being applied too, in this case it's link, here are some settings for Core -> Base -> Forms. Settings for anything in Utilities, Objects, and Components do the same but also are prefixed with either u-, o-, and c- respectively. But the global settings here start with the most common part of what they're applying, so for the colour settings its: $color-. All settings should be in context i.e. in the relevant partials, but the global settings can't.

These dark and light overlay modifiers don't look right https://github.com/chris-pearce/scally/blob/v2/objects/_o-overlay.scss#L142. Classes on silent placeholders?

I'll look into it, cheers.