Unencapsulate css global reset

[tinynumbers]

Several people have started using this fork in order to get around the issues reported in #1852.

This pull request effectively rolls back #1343 and closes #1852.

[tinynumbers]

Travis is apparently having issues testing out pull requests? In any case, this passes all tests run locally.

[seanlinsley]

Our Travis builds on master are now green (as of 166bc46). Please rebase your PR on the current master:

# Expectations: "upstream" is gregbell's GitHub repo and "origin" is your fork

# Rebase your fork's master branch with the latest upstream changes
git checkout master
git pull --rebase upstream master
git push origin master

# Rebase your feature branch with the latest upstream changes
git checkout your_feature_branch
git pull --rebase upstream master
git push origin your_feature_branch # note that you may need to use -f

[tinynumbers]

Rebase done... Thanks for the tip.

[seanlinsley]

You should have followed the instructions. You want to rebase, not merge the changes.

[seanlinsley]

What you want to do now is:

git stash
git reset --hard 55ef578 # reset to your latest commit before the merge
git stash pop

And then run

git pull --rebase upstream master

[tinynumbers]

Like so?

[seanlinsley]

Yep.

Still waiting for some input from other maintainers: @macfanatic, @pcreux, @gregbell

[macfanatic]

@daxter - Greg is out of pocket for the next 2 weeks. I see the reasons mentioned in #1852, but I'm not sure of the best way to fix either. I'm not convinced reverting this change is a good move, or one that at least should be reserved for the 0.6 release since it is an important change.

[seanlinsley]

Yeah I know Greg's away. Still wanted to ping him in any case.

I was there when @ronen first proposed #1343, and at the time it made sense. But rereading that thread, it seems to me that the fundamental reason for the PR itself was a non-issue.

In the PR @ronen says that namespacing the CSS was intended to prevent AA rules from overriding rules from your application. But that shouldn't be an issue, because active_admin.css.scss shouldn't be loaded up in your application in the first place.

Meanwhile, the changes resulted in plugins like CKEditor breaking because in CSS, higher specificity means higher priority. Since every AA CSS rule has the body.active_admin specifier, they override any 3rd party styles.

See what I'm getting at?

Theoretically you should just be able to do this:

body.active_admin {
  @import "stuff";
}

but that makes the assumption that the 3rd party styles use SASS, and also prevents the 3rd party styles from configuring the body HTML element. That might be resolved by using html.active_admin instead, but the SASS prerequisite is still there.

[brodock]

👍

[tinynumbers]

Any updates for this?

It continues to cause problems, see for example kreativgebiet/rich#62

[seanlinsley]

@tinynumbers I'm convinced that this should be merged, but I'm still waiting on input from the rest of the team.

[ncimino]

Any updates on this?

[mindhalt]

👍 Desperately need this. I can't normally integrate CKeditor and Chosen without it 😢

[kars7e]

Same here. Please merge it!

[socjopata]

I would love to see it updated.

[mwlang]

+1

[seanlinsley]

Comments @gregbell, @macfanatic, @pcreux?

I think this should be part of the 0.6.1 release.

[brodock]

+1 please do it!
the way it is right now is obviously broken and the fix obviously works as many many others are using the fork with success (i include myself on those)

[mindhalt]

@daxter will you include it into rails4 branch? :)

[seanlinsley]

Just did. aacd755 -> 54fb71c

[smidwap]

I've been using active_admin on Rails 4 for about a month now and just today updated the gem. I had a file app/assets/stylesheets/active_admin.css.scss that got automatically included in my application.css upon precompilation, so now the AA styles are overriding my styles. What's the recommended way to only include active_admin.css.scss in the admin pages?

[smidwap]

Or rather, what's the recommended way to not include active_admin.css.scss in application.css?

[seanlinsley]

That would be by removing the require_tree directive.

[smidwap]

@daxter Thanks for the quick response. Is that considered best practice? It's not a huge deal, but it's nice not to have to explicitly require each of my app's stylesheet.

[seanlinsley]

It looks like it's possible to explicitly blacklist certain files from the asset manifest:

//= stub active_admin

[smidwap]

Brilliant, thanks @daxter

[seanlinsley]

No problem. Glad you asked that question, since it lead me to discover stub 🐙

It might be a good idea to auto-add a stub to projects that install AA. It would save people a lot of trouble at the outset.

[smidwap]

Based on some funny behavior Just did some more research on that issue @daxter. The PR for sprockets that added stub specifically points out that stub should NOT be used to "exclude" assets. It's confusing why, but it's pretty clear said here:

sstephenson/sprockets#202

When I tried using //= stub active_admin in my application.js file, stub went a step further and removed jquery and the other assets required in active_admin.js.

There's no feature in sprockets to actually do what we need:

sstephenson/sprockets#86

What might be better is to place active_admin.css in a sub-directory, so it could be active_admin/active_admin.css. Then developers could be urged to change require_tree . to require_directory .

[smidwap]

Then again, stub might "accidentally" work for application.css because active_admin.css doesn't have any require's

[seanlinsley]

@smidwap this obviously requires further discussion. Could you create a new Issue for it?