Nested field sets need some love.

Nested Fieldsets

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Status: Active » Closed (won't fix)

Sorry, this was omega 4's doing...

joelpittet’s picture

Project: Ember » Omega
Version: 7.x-2.x-dev » 7.x-4.x-dev
Status: Closed (won't fix) » Active

Moving to Omega.

joelpittet’s picture

Status: Active » Needs review
FileSize
1002 bytes

Ok, so personally I think the styles for this can go because it gets to greedy the way it adds styles to a page that belongs to the admin theme. So maybe there was a reason to add these styles but the patch provided drops them out. I'd personally recommend dropping most of the styles in this omega.admin.css file if possible to avoid conflicts with admin themes. Sorry for sounding preachy, hope this falls in line with what you guys are doing.

msmithcti’s picture

Status: Needs review » Fixed

So, I don't think simply removing the styles is the best thing - they are there for a reason. I don't think either of us have looked at that file for a while and it definitely needed some attention!

Just pushed 6db788c that refractors the admin styles. They should now be easier to override and the selectors more targeted which should reduce the "greed"! Checked the changes over with Wraith which shows these changes have no visual affect.

Also if the file is still interfering, don't forget the it can just be excluded :)

joelpittet’s picture

Thanks @splatio, that commit does help, though I think someone may have forgot to compile the CSS on that commit?

fubhy’s picture

We never add compiled CSS in the same commit. Putting that in a separate commit keeps the git log clean. Sadly we do have to include the CSS. For my client projects I am at a point where I don't commit the CSS at all and instead have it get built as part of my continuous integration / delivery setup. However, for drupal.org module/theme packaging that doesn't work :( (hence the separate commit, which is better than just having a single commit for both, the source .sass and the compiled .css).

joelpittet’s picture

@fubhy that is very interesting approach. Can I ask how it makes the git log clean? To me it just sounds like twice the amount of work to make separate commits for every CSS change.

I can see the cache busters on images getting annoying if the image files didn't change.

fubhy’s picture

Can I ask how it makes the git log clean? To me it just sounds like twice the amount of work to make separate commits for every CSS change.

Well, it makes the diff's readable. Compiled stuff in VCS is never readable and completely screws up the diff.

Hence also why I always ask for patches provided in the issue queue to not contain the .css changes.

Doing a second commit after a grunt dist build just takes about 10 seconds. Not sure if I would call that extra work :)

joelpittet’s picture

Thanks for the reply @fubhy, I agree totally with compiled stuff in any VCS is a bad idea. Though in drupal's case couldn't you leave the "compile" minification/aggregation to Drupal and at most you have a duplicate CSS version of the SASS which is easy to read in the diff, saving you a step that may get missed by mistake in the process, and saving you from explaining not to include the .css in the patches? Just a thought.

Though maybe in D8 if we can get assetic in, the sass compile can be included in the module... *crosses fingers*

{% stylesheets '@omega/sass/omega.admin.scss' filter='sass,?yui_css' output='css/omega.admin.css' %}
    <link href="{{ asset_url }}" type="text/css" rel="stylesheet" />
{% endstylesheets %}

Or something equally unicornish.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.