Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Defining CSS reset within body.active_admin clobbers third party gem styles #1852

Closed
pietschy opened this Issue · 23 comments

10 participants

@pietschy

From what I can see, since #1343 AA targets it's CSS resets styles to body.active_admin. This is giving them higher specificity than styles included later in the asset chain.

This is a big problem when using third party plugins such as https://github.com/bastiaanterhorst/rich. The the resets completely clobbering things and I have to do various ugly hacks to make things work.

A quick fix of moving the reset out of body.active_admin might work but I think the larger problem is that the common pattern '... is to include all css files together.' (mentioned in the pull request) isn't really going to work here.

I'm not a rails expert but I would have thought it was possible to include AA specific assets on only AA pages so the body.active_admin selector can be dropped???

@lmb

I've had a look at this issue and unfortunately the fix isn't entirely trivial, since active admin as well as upstream vendors will have to make changes.

Specifically, have a look at lmb/active_admin@5f4811f. Making AA's reset less specific fixes some of the issues:
CKEditor with less specific CSS reset

Unfortunately, margin and padding is missing on some elements, since CKEditor does not sufficiently scope their .cke_top and .cke_toolgroup classes (at least for the new Moono skin).

I'm not opening a pull request yet, since I'd like some more input on whether this is the right way to go. Specifically, does somebody know if .active_admin is used on elements other than <body>? Also, I'm not certain if the weird lines around the top and bottom of the CKEditor icons are the same problem or if it's a different issue.

@seanlinsley
Owner

Just confirming, you tried this, right?

body.active_admin {
  // Mixin from Active Admin: _buttons.css.scss
  a.cancel, a.clear { @include light-button }
}

By that I mean, are you including the CKEditor CSS inside the body.active_admin selector?

@lmb

I'm not sure what you intend with this code snippet, but if I understand correctly you're referring to the fix at the bottom of this document? I went that route, which has two drawbacks for me:

  1. I have to copy over all .css to .scss in my application folder everytime the galetahub/ckeditor gem is updated, which is a) cumbersome and b) not really the place they belong.
  2. It breaks full-screen editing (using the maximize plugin). Which is partially the plugins fault.

The problem with this issue is, that both AA and CKE can fix this. Which makes it no one's problem.

@seanlinsley
Owner

Specific to your situation, this is what I meant.

body.active_admin {
  @import "ckeditor/application";
}

Though I agree we should remove the body specification. Reason being, it resulted in this CKEditor rule:

body.active_admin body {
  padding: 0;
  margin: 0;
  font-family: Arial, Helvetica, sans-serif;
}

Which obviously wouldn't match anything.

@pietschy pietschy referenced this issue in kreativgebiet/rich
Closed

ActiveAdmin 0.5.0 clobbering editor styles #62

@pietschy

Hi guys,

Thanks for the input.

@Daxter
Yep, I have been fixing this by putting the styles under body.active_admin but it's a pretty ugly hack and is completely non DRY. That and I have to do it for every 3rd party widget I'm using.

My main point for the issue is that I think using the body.active_admin scope is not the best way to solve the problem for which it appears to have been added (i.e. to stop AA styles from clobbering styles in non AA portions the websites). I think a better approach is to not include AA styles on non AA pages.

@Imb
I just tried the nested @import "ckeditor/application" but it didn't seem to work for me. I didn't look too hard as to why though since as you mentioned it's can create non sensical style rules (and I've gone with the approach of manually re-defining the clobbered styles for now) . Also prior to SASS 3.2 that line would break if the import defined any mixins.

@lmb

@Daxter
I was under the impression that importing non-scss files in SASS is currently not possible? That was one solution I was thinking of. It would make a workaround for these kinds of bugs easy and very low maintenance. (Put css in vendor/ and @import them in a custom stylesheet in lib/ or sth.)

@pietschy
See my comment above. Just had a look, and this as well as this issue is what I am talking about.

In the meantime, any chance that the CSS 'fix' I referenced will go into mainline? I think it'd be a good idea regardless.

@seanlinsley
Owner

@lmb, CKEditor uses SASS the rules are included nonetheless. If you open up the generated CSS file from your server and search for "ckeditor", you'll see what I mean.

@pietschy pietschy closed this
@pietschy pietschy reopened this
@pietschy

Sorry, I clicked close accidentally.

@lmb

@Daxter
Thanks for the info, I'll check this out. Is that some undocumented behaviour then?

@tinynumbers
From what I understand, the problem doesn't lie with ActiveAdmin. It's the default sprockets config that includes AA's reset stylesheet. You'll have to either tell everyone that uses AA to change that default config or try to scope the reset as to be ineffective on non AA pages. Which I think is reasonable.

@pietschy

@imb
I'm fairly new to rails, but I don't think the styles are included automatically. The AA reset is contained in /active_admin/_base.css.scss which is imported by app/assets/stylesheets/active_admin.css.scss. If I remove that line from my app all the AA styles go away. From what I understand sprockets only includes a file (or files) if it's included via a manifest or sass import directive.

Hence my contention that the body.active_admin is not required since the premise of the original but report #1343 was wrong. If you don't want the AA styles you shouldn't include them.

Unless I'm wrong of course (c:

@tinynumbers

Hence my contention that the body.active_admin is not required since the premise of the original but report #1343 was wrong. If you don't want the AA styles you shouldn't include them.

I guess I'm agreeing here - and corollary to this, if you want the Active Admin styles scoped by body.active_admin, then do it yourself, but I don't believe the project itself should impose this, since wrapping everything in a body.active_admin tag gives every style in the AA stylesheets a higher precedence than any style of any third party plugins (e.g. CKEditor) that we might want to use in conjunction with AA.

So ideally #1343 would be rolled back and replaced with tweaks to the CSS that allow all of the AA styles to be encapsulated by anyone who wants to do so, but does not impose this.

In the meantime I'm going to be using my fork at https://github.com/tinynumbers/active_admin/tree/unencapsulate-css-global-reset until this gets resolved. Anyone else is free to do so as well. I'll be doing my best to keep it in sync with the master (other than the CSS encapsulation).

@pietschy

@tinynumbers,
Thanks for the fork. I'm stuck with using my own as I have other tweaks. Mine only removes the class around the reset but that worked enough for me. If I can figure out how to pull your changes in I'll give it a go. Here's how Ckeditor looks now.
Screen Shot 2013-01-30 at 6 44 23 PM

@pcreux, @gregbell
What would be required to get this changed back?

@ArthurN

Thank you for that fork, @tinynumbers. That did the trick for me, and I haven't seen it affect anything else (including my custom styles and other 3rd party styles). Cheers.

@Mayar

+1 to remove body.active_admin! So annoying...

@mark9white

Another +1 from me, using @tinynumbers fork for now ... (thanks for providing it @tinynumbers )

@tinynumbers

For those using my fork to get around the body CSS encapsulation, note that I just merged in the latest changes from the main repository master branch. A number of new features in there, so be warned...

@HuffMoody

Why don't we just remove the body.active_admin encapsulation all together and then mention in the Readme that require_tree in application.css will cause conflict with active admin. require_tree is not good practise anyways and shouldn't be used; there is no sense including the entire active_admin js/css files into the main application js/css just for convenience of setup.

@seanlinsley
Owner

The require_tree problem has always been a problem, regardless to this issue.

We're currently waiting on feedback from @gregbell over in #1952, so this is on hold for now.

@brodock

:+1: I'm using it for sometime and it works pretty well

@mwlang

I ported @tinynumbers patches to rails4 branch. Should definitely avoid polluting the body tag as I struggled with ckeditor, tinymce editor, rich editor, and a few others I was trying before I realized it was Active Admin that was messing things up rather than the wysiwyg editors themselves.

patched_view

@seanlinsley seanlinsley closed this in #1952
@seanlinsley
Owner

This has now been resolved on master. Everyone, sorry that it took so long to fix this.

@mwlang

Excellent! This fixes the issues I was having.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.