Skip to content
This repository has been archived by the owner on Aug 5, 2020. It is now read-only.

Add a desktop override mixin #31

Merged
merged 10 commits into from
Sep 29, 2014
Merged

Add a desktop override mixin #31

merged 10 commits into from
Sep 29, 2014

Conversation

ry5n
Copy link
Contributor

@ry5n ry5n commented Sep 3, 2014

On level 1 sites we often need to style on top of some truly gnarly existing CSS. This mixin helps overcome extreme speficicity while otherwise allowing us to maintain our normal CSS standards. It also avoids the temptation to use !important for this purpose, keeping it in reserve for when it’s really necessary.

Status: Ready to merge

Reviewers: *@kpeatt @jeffkamo @tedtate *

Changes

  • Adds a new (re-written) version of the experimental desktop-override mixin, now called style-override. This version was developed after running into limitations with the old version on an actual client project.
  • This version is simpler (relying on the ability to chain the same id selector to itself as many times as needed).
  • It also works in more scenarios, being able to style any element within <body>, not just within <div id="x-root">.
  • The only requirement is a consistent id attribute of ! on the <head> tag across all pages: <head id="!">. This id was chosen to be short (since it’s prepended n times to every selector) and obscure (it’s a special character and must be escaped for use in CSS or JS).

How to test-drive this PR

  • Check out this branch and run grunt in the root dir, then open the test html file and review the test for this mixin.

@ry5n
Copy link
Contributor Author

ry5n commented Sep 3, 2014

Is there any risk in forcing a particular id on the head tag? Anyone know of a scenario in which an existing id might be present and require changing the selector that this uses?

@kpeatt
Copy link
Contributor

kpeatt commented Sep 3, 2014

I like this id on head idea. When would you ever have one? I can think of no scenarios. If we absolutely must, maybe we let the selector be passed as a variable to the mixin.

@ry5n
Copy link
Contributor Author

ry5n commented Sep 3, 2014

@kpeatt That’s what I was thinking too.

Oh, and one more thing: I realized what we should call this:

the head-bang hack.

Beavis and Butthead headbang

@kpeatt
Copy link
Contributor

kpeatt commented Sep 3, 2014

Lool.

@kpeatt
Copy link
Contributor

kpeatt commented Sep 3, 2014

Let's add the ability to specify selector and then I think this is GtM™.

@ry5n
Copy link
Contributor Author

ry5n commented Sep 3, 2014

OK, I’ll add that in.

@jeffkamo
Copy link
Contributor

jeffkamo commented Sep 4, 2014

The way it's written, I think you have to use the include completely unnested. Is it possible to be written in a way that it can be used nested?

Example...

.t-pdp {

    @include style-override(1) {
        .client-class {}
    }
}

Which I believe outputs...

.t-pdp #\! ~ body .client-class {}

@ry5n
Copy link
Contributor Author

ry5n commented Sep 4, 2014

The only way this works is if the template classes are added on <html> instead of something else. Otherwise, I don’t think there’s a way around this, although maybe some Sass magic with the new @root directive which I haven’t really used yet.

@ry5n
Copy link
Contributor Author

ry5n commented Sep 4, 2014

Of course, you can get around it by either wrapping the mixin around all of .t-pdp or you can do something like this:

.t-pdp { /* styles */}

@include style-override() {
    .t-pdp { /* styles requiring override */ }
}

In a long template stylesheet this could become confusing. On EBT I’ve just been wrapping the entire partial.

@kpeatt
Copy link
Contributor

kpeatt commented Sep 4, 2014

@ry5n I rewrote this to use @at-root and made a function to check if we're currently inside a nest or not. This forces Sass 3.4 compatibility so I guess we have to decide if that's worth it.

@ry5n
Copy link
Contributor Author

ry5n commented Sep 4, 2014

Changes here look good. Question depends on how quickly we want to go to 3.4 and deal with the new variable scope etc.

@kpeatt
Copy link
Contributor

kpeatt commented Sep 5, 2014

@ry5n I don't think the new variable scope is anything to be concerned about. Biggest question for me is whether we can lock dependencies in the future.

@ry5n
Copy link
Contributor Author

ry5n commented Sep 5, 2014

OK cool. Let’s wait to see what T and co. can do on that front before committing to this.

@kpeatt
Copy link
Contributor

kpeatt commented Sep 8, 2014

@tedtate Thoughts on this?

@kpeatt kpeatt modified the milestones: Spline 1.0, Post Spline 1.0 Sep 8, 2014
donnielrt added a commit that referenced this pull request Sep 29, 2014
@donnielrt donnielrt merged commit e0bba6a into master Sep 29, 2014
@donnielrt donnielrt deleted the desktop-override-mixin branch September 29, 2014 18:43
@kpeatt
Copy link
Contributor

kpeatt commented Sep 29, 2014

@ry5n Just going ahead and releasing this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants