Screenshot of the issues.

This commit seems to have broken the fieldset collapsed/expanded png icons.

The bottom margin issue was present before beta11.

CommentFileSizeAuthor
#24 seven-fieldsets-user-register.gif3.52 KBalan d.
#24 seven-module-listing.gif17.29 KBalan d.
#14 seven-block-inputfilters-issue.png87.78 KBmccrodp
#9 seven-877386-9-img-paths.patch680 bytesfenstrat
#5 seven-877386-6-fix-fieldsets.patch3.21 KBfenstrat
#2 seven-fieldsets-before.png6.62 KBdixon_
#2 seven-fieldsets-after.png4.16 KBdixon_
#2 877386-fieldset-margin-2.patch658 bytesdixon_

Comments

mcrittenden’s picture

mcrittenden commented
Component: Code » User interface
Assigned: Unassigned » mcrittenden

Dang.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented
Version: 6.x-1.0-beta11 » 6.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new 877386-fieldset-margin-2.patch658 bytes
new seven-fieldsets-after.png4.16 KB
new seven-fieldsets-before.png6.62 KB

Here is a patch that fixes this problem and screenshots illustrating the result.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs review » Needs work 
+++ style.css	4 Sep 2010 14:40:07 -0000
@@ -572,7 +572,7 @@ fieldset {
+  margin: 0.5em;

Nitpick, but shouldn't this be margin: 0.5em 0; for consistency with the change below?

Additionally it doesn't fix the first issue noted in this screenshot, i.e. menu-(collapsed|expanded).png is still missing.

lpalgarvio’s picture

lpalgarvio commented

Fieldset Helper has trouble with this theme:
http://drupal.org/node/902646

the collapse and uncollapsed all buttons do not work.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs work » Needs review
StatusFileSize
new seven-877386-6-fix-fieldsets.patch3.21 KB

Screen shots of:

The issue in ie
The issue in firefox
The solution with this patch applied

A good place to test this patch is on admin/user/settings

All the changes I've made come directly from seven in D7. Or they are required changes to accommodate D6 -> D7 differences. I also included the one needed change from #2.

I've tested the results in Win: IE 6, 7, 8, Firefox, Chrome, Safari, Opera - Ubuntu: Firefox, Chrome, Opera

@LPCA I have no experience with the fieldset_helper module, but as seven overrides fieldset markup in seven_fieldset() I'd say that could be the cause of the issue.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Reviewed & tested by the community

The patch looks good and I confirm it fixes fieldsets on:

Win: IE 6, IE 7, IE 8, Firefox 3.6, Chrome 6, Safari 5, Opera 10.6
Mac: Safari 4.1, Firefox 3.6

mcrittenden’s picture

mcrittenden commented
Status: Reviewed & tested by the community » Fixed

Committed to beta12. Thanks guys/gals!

lpalgarvio’s picture

lpalgarvio commented

looks like it's fixed ;)

fieldset_helper however still doesn't work well, but the developer states is a problem on his module.
i'll inform him of the changes.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Fixed » Needs review
StatusFileSize
new seven-877386-9-img-paths.patch680 bytes

Here's a quick follow up to make the paths to images in the misc folder consistent as per #927004: Wrong paths for menu-expanded.png and menu-collapsed.png - this makes all misc paths consistent.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

@fenstrat: that doesn't work for when you have the theme in a directory other than sites/*/themes/seven, but I will agree is a better patch than what I had originally provided in #927004: Wrong paths for menu-expanded.png and menu-collapsed.png.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented

@DamienMcKenna, "/misc/an-image.png" works wherever the theme lives, however as mcrittenden noted it doesn't work if Drupal isn't in the webroot (e.g. installed in a sub dir).

As for the "../../../../misc/an-image.png" approach in #9 (the same as your approach in #927004), as you said, it doesn't work unless seven is in "sites/*/themes/seven". However it's pretty unlikely seven will live anywhere else.

So both work, but there's trade off's in both cases. I'd say go with the 2nd option as per #9.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

I use seven in the profiles/[profile]/themes/seven directory, this change would work for me, but I think it can be fixed with a subtheme anyway.

lpalgarvio’s picture

lpalgarvio commented

another issue came up with the Util module, when using the Permission fieldset submodule.

i dont know if its a problem on it or a problem in the theme, so i also reported it in the util module.
http://drupal.org/node/958206

mccrodp’s picture

mccrodp commented
StatusFileSize
new seven-block-inputfilters-issue.png87.78 KB

Hi,

I'm getting something similar but different on the newest beta13 version of seven for 'INPUT FILTERS' on blocks. It is different as it doesn't even show the 'INPUT FILTERS' legend text unlike in the screenshot attached by fenstrat in #3 above.

If this is a separate issue I'll post a separate bug, but it seems similar to me. I have attached a screengrab showing the missing field and the firebug display. You can also see the 'display: none' on the element itself in the CSS file vertical-tabs.css.

Looks like an issue to me.

Thanks,
~mccrodp

EDIT - Changing to 'display: block' did nothing, this issue looks similar to http://drupal.org/node/312749 on panels

mrfelton’s picture

mrfelton commented
Status: Needs review » Fixed

I believe tese fieldset issues should be resolved in the latest code in git.
http://drupalcode.org/project/seven.git/blobdiff/e8d43797881420bb8f99e58...

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Fixed » Reviewed & tested by the community

Thanks mrfelton for the commit in #15.

However the patch here in #9 seems to have been overlooked. It simply makes image paths to /misc/*.png consistent with what was committed in #927004-1: Wrong paths for menu-expanded.png and menu-collapsed.png

mrfelton’s picture

mrfelton commented

@fenstrat: I'm not really convinced that is the appropriate route to take with the path issue, since there is no guarantee that the theme will be located 4 levels deep from the Drupal root directory. For example, I tend to work with drush make files, aegir and installation profiles, and I usually store contributed themes themes in /profiles/profilename/themes/contrib/seven. This would not work with the adjustment you proposed, as it assumes the theme is located in the rot of the profile's theme directory.

mikeytown2’s picture

mikeytown2 commented

Another idea is to copy misc/menu-expanded.png into the seven directory. Not ideal but it does solve the issue.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented

@mrfelton: I fully appreciate that it is not an ideal solution, all it's trying to do is make it consistent with what was already committed to the styles.css file.

mikeytown2's idea of copying the png's into the seven directory is one sure way to solve the issue.

mrfelton’s picture

mrfelton commented
Status: Reviewed & tested by the community » Fixed

Ok, since I can't think of a better way of doing it, I've added the two images from /misc into the theme and adjusted the css to reference them from there. Since Seven's primarily an admin theme, I think we can get away with this. If it were a user facing theme I'd have to think twice since we would then be talking about lots of users downloading duplicate images.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented

Great, I agree with the reasoning in #20.

What was actually committed: http://drupalcode.org/project/seven.git/commit/c70dc2c

Status: Fixed » Closed (fixed)

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

alan d.’s picture

alan d. commented

Wanna release a new version with this fix?

alan d.’s picture

alan d. commented
Status: Closed (fixed) » Needs work
StatusFileSize
new seven-module-listing.gif17.29 KB
new seven-fieldsets-user-register.gif3.52 KB

Actually, there are still fieldset issues in FF 5, see screenshots. (Using master)

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs work » Closed (fixed)

@Alan D. Please open a new issue.

alan d.’s picture

alan d. commented

With the exception of the icons, the FF issues are exactly the same as the initial bug description.