I'd like to suggest we align the layout to a 12 col grid to bring visual consistency and simplify the layout CSS, imo this makes it easier to change and extend.

The patch does not use a framework, I've only changed the widths and padding, and removed all left and right margins from sidebars etc.

This change also makes better use of the horizontal space, especially when two sidebars are active (375px before, 460px after).

This patch totally rewrites both layout.css and layout-rtl.css, stripping around 30 lines from each.

Attached a screen-shot to show the grid.

CommentFileSizeAuthor
#23 bartik_layout_30px_gutters_v1.patch1.03 KBJeff Burnz
#23 bartik_30px_gutters.png72.3 KBJeff Burnz
#18 issue749062-drhino-40px_col_adjustment.patch540 byteswatsonerror
#9 bartik_layout_align_to_grid_v1.2.patch3.7 KBJeff Burnz
#7 bartik_layout_align_to_grid_v1.1.patch3.7 KBJeff Burnz
#2 bartik_layout_align_to_grid_v1.0.patch3.54 KBJeff Burnz

Comments

jensimmons’s picture

jensimmons commented

I think the screenshot didn't make it onto this issue.

Jeff Burnz’s picture

Jeff Burnz commented
StatusFileSize
new bartik_layout_align_to_grid_v1.0.patch3.54 KB

I think d.o might be overloaded, I'm getting 503 errors for past few hours and having issues with attachments, trying again for the patch.

Jeff Burnz’s picture

Jeff Burnz commented

Now I'll try with the screen shot... ok, so that didnt work, trying something different...

Messed about with colors to show the grid better.

Only local images are allowed.

Jeff Burnz’s picture

Jeff Burnz commented
Status: Active » Needs review

Set status now there's a patch.

joachim’s picture

joachim commented
Status: Needs review » Needs work

This looks like a good improvement to make.

The patch fails to apply on several counts:

- please roll patches from the top level of a project; so in this case, stand inside /bartik when you do cvs diff.
- once I move into the css folder, I get a failed hunk for layout-rtl.css

Also, Dreditor review flags up the lack of a newline at the end of the files, though this is present in bartik's layout.css as it currently stands -- hence not up to this patch to fix.

Jeff Burnz’s picture

Jeff Burnz commented

Noted, apologies, I rolled on the fly without thinking about it, will reroll.

Jeff Burnz’s picture

Jeff Burnz commented
Status: Needs work » Needs review
StatusFileSize
new bartik_layout_align_to_grid_v1.1.patch3.7 KB

New patch, this one should be good to go.

joachim’s picture

joachim commented

Looks great.

If I wanted to nitpick I'd say the main menu's first item could have its left edge aligned with the grid too... but that's a minor point whose aesthetics are debatable...

Patch gets a +1 from me :)

Note that it still hasn't been rolled from the right place; to apply it you need to say p4 rather than p0.

Jeff Burnz’s picture

Jeff Burnz commented
StatusFileSize
new bartik_layout_align_to_grid_v1.2.patch3.7 KB

@joachim I agree about the menu, we can attack that in the next patch, unfortunately I don't have time right now to fully test changes to the header - I was planning on attacking that next to align all elements in the header etc.

Can you check this patch, I read up about patches and I take you are referring to the -p option prefix containing num leading slashes from each file name found in the patch file (its all gibberish to me), so I spent some time looking at other patches and messing in Eclipse so I think this is what you mean (see patch). Cheers.

joachim’s picture

joachim commented

If that is the identical changes as before with just the rolling method different, then yes, the filenames look right now.

(The 'prefix containing num leading slashes...' bit is largely gibberish to me too... it's to do with the patch command turning 'foo/bar/biz/bax.php' into just 'biz/bax.php' and to do that it strips two slashes.)

jensimmons’s picture

jensimmons commented
Status: Needs review » Needs work

#749062 by Jeff Burnz | joachim: Added Align to grid, simplify layout.css and RTL.

There are problems with this, imo. I spent weeks looking at a grid on Bartik last fall, and never made up my mind about what to do. (That's why the extra space was hanging around.)

For one, the triptych columns end up too crowded.

Only local images are allowed.

Also, the size of the image cache preset for the images needs to be taken into consideration.... and there's more.

But meanwhile, I think what you've done Jeff is a great step forward, especially now that we have two columns. So I'm going to:

a) Commit this.

and

b) Leave this issue open.

This needs more work. It's gone from 50% there to 80% there. :)

jensimmons’s picture

jensimmons commented
Title: Align to grid, simplify layout.css and RTL » Adjust layout (deviating a bit from the grid) to make things look better.

Changing the title too.

Jeff Burnz’s picture

Jeff Burnz commented

Awesome, OK, I'll keep working on it, I agree the triptych columns need more spacing - and what is really needed here is a more generic way of adding extra space or some special casing (which is probably OK given its just this one set of boxes).

jensimmons’s picture

jensimmons commented
Title: Adjust layout (deviating a bit from the grid) to make things look better. » Adjust grid to put more space between things.

I think we should consider using a grid with 40 or 50px between columns, rather than a grid with 20px between columns. 20px gutters make sense for sites with a tremendous amount of content on each page, with small type and lots of teasers. Bartik, however, is designed to work without a lot of content. Anyone who wants to use it with lots of content can tweak the CSS to fit more stuff.

I realized this when looking at the polls next to the sidebar. It's too crowded, especially given how sparse the rest of the space is.

Only local images are allowed.
http://img.skitch.com/20100405-q836niqe99btcyk2147ajkf39j.jpg

Only local images are allowed.
http://img.skitch.com/20100405-xni1h5mgbad87p9x8694h9yfka.jpg

jensimmons’s picture

jensimmons commented
Title: Adjust grid to put more space between things. » Adjust grid to put more space between things

I just put a 40px gutter in place for the Triptych so that I could work on the theming for it:

#triptych-first,
#triptych-middle,
#triptych-last {
  margin: 20px 20px 30px;
  width: 280px;
}
watsonerror’s picture

watsonerror commented
Assigned: Unassigned » watsonerror

nothing to say

bleen’s picture

bleen commented
Priority: Normal » Critical

we need to fix this before officially submitting Bartik for consideration

watsonerror’s picture

watsonerror commented
Priority: Critical » Normal
Status: Needs work » Needs review
StatusFileSize
new issue749062-drhino-40px_col_adjustment.patch540 bytes

Changed left and right column widths to 40px, it's kinda wide.

bleen’s picture

bleen commented
Status: Needs review » Reviewed & tested by the community

this works fine :)

bleen’s picture

bleen commented
Priority: Normal » Critical

priority got changed during cross-post

bleen’s picture

bleen commented
Status: Reviewed & tested by the community » Needs work

Let me back up a second on the review of the patch in #18...

The patch submitted works fine, but I think there are still other places where the grid layout needs to be fixed. For example, inline images within a node need to have more spacing around them to adhere to the grid

joachim’s picture

joachim commented

> For example, inline images within a node need to have more spacing around them to adhere to the grid

I think they're outside the problem, as there's no way to know what size images a user puts into a node.

More generally: I think 20px is bearable, but I see jensimmons' point that it's too cramped. 40px is going to be a bit narrow for layouts with 3 columns, but in general it looks fine.

Jeff Burnz’s picture

Jeff Burnz commented
StatusFileSize
new bartik_30px_gutters.png72.3 KB
new bartik_layout_30px_gutters_v1.patch1.03 KB

I'd like to propose we use 30px gutters, all round - including the footer columns and triptych (with 5px extra margin to get the 40px).

The patch removes the guff special casing for footer cols - I think this is more consistent and works well, 40px is too much imo and robs the theme of valuable horizontal space.

I'm not totally against the 40px (well, I am actually) but more so against special casing - this just makes it harder for neophytes to make changes as suggested, if they can change one padding value and the whole thing changes its just easier. Basically half the point the of the original patch was to simplify the layout, esp for RTL.

Granted there are things that need work wrt to the grid - the logo and other elements need some adjustment (the whole footer for one is a big odd and needs work), but I think for this patch lets stick to just the layout, fix and repair the other stuff once we have this sorted.

Jeff Burnz’s picture

Jeff Burnz commented
Status: Needs work » Needs review

dam status.

jensimmons’s picture

jensimmons commented

Ok — I'm convinced to go with 30px. Is the patch in #23 complete? I'm going to look at committing it right now.

I think the footer columns should stay with 20px gutters.

jensimmons’s picture

jensimmons commented
Assigned: watsonerror » Unassigned
Status: Needs review » Closed (works as designed)

Alrighty. I made sidebar and content have 30px gutters. Left Triptych with 40px gutters. Left Footer cols with 20px gutters. Did not move things around so that one element sets the beginning padding and a second modifies it (which I think is way more confusing for front-end devs who hack this theme).

I'm also now softly bringing down the hammer on this and considering it a closed issue. I've thought long and hard about this tonight, and this is what I want to do.

Committed.