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

Unencapsulate css global reset #1952

Merged
merged 2 commits into from Jul 8, 2013

Conversation

Projects
None yet
10 participants
@tinynumbers
Contributor

tinynumbers commented Feb 28, 2013

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

This comment has been minimized.

Contributor

tinynumbers commented Feb 28, 2013

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

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Mar 15, 2013

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

This comment has been minimized.

Contributor

tinynumbers commented Mar 15, 2013

Rebase done... Thanks for the tip.

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Mar 15, 2013

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

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Mar 15, 2013

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

This comment has been minimized.

Contributor

tinynumbers commented Mar 15, 2013

Like so?

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Mar 15, 2013

Yep.

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

@macfanatic

This comment has been minimized.

Contributor

macfanatic commented Mar 15, 2013

@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

This comment has been minimized.

Member

seanlinsley commented Mar 15, 2013

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

This comment has been minimized.

brodock commented Apr 23, 2013

👍

@tinynumbers

This comment has been minimized.

Contributor

tinynumbers commented May 17, 2013

Any updates for this?

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

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented May 17, 2013

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

@ncimino

This comment has been minimized.

ncimino commented May 31, 2013

Any updates on this?

@mindhalt

This comment has been minimized.

Contributor

mindhalt commented Jun 9, 2013

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

@kars7e

This comment has been minimized.

kars7e commented Jun 16, 2013

Same here. Please merge it!

@socjopata

This comment has been minimized.

socjopata commented Jul 3, 2013

I would love to see it updated.

@mwlang

This comment has been minimized.

mwlang commented Jul 5, 2013

+1

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Jul 5, 2013

Comments @gregbell, @macfanatic, @pcreux?

I think this should be part of the 0.6.1 release.

@brodock

This comment has been minimized.

brodock commented Jul 8, 2013

+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)

@seanlinsley seanlinsley merged commit 6238c12 into activeadmin:master Jul 8, 2013

1 check passed

default The Travis build passed
Details
@mindhalt

This comment has been minimized.

Contributor

mindhalt commented Jul 8, 2013

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

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Jul 8, 2013

Just did. aacd755 -> 54fb71c

@smidwap

This comment has been minimized.

smidwap commented Jul 9, 2013

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

This comment has been minimized.

smidwap commented Jul 9, 2013

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

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Jul 9, 2013

That would be by removing the require_tree directive.

@smidwap

This comment has been minimized.

smidwap commented Jul 9, 2013

@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

This comment has been minimized.

Member

seanlinsley commented Jul 9, 2013

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

//= stub active_admin
@smidwap

This comment has been minimized.

smidwap commented Jul 9, 2013

Brilliant, thanks @daxter

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Jul 9, 2013

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

This comment has been minimized.

smidwap commented Jul 9, 2013

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

This comment has been minimized.

smidwap commented Jul 9, 2013

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

@seanlinsley

This comment has been minimized.

Member

seanlinsley commented Jul 9, 2013

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

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