Code Review Request

HTML & CSS
1

Over the last couple of days, I have been working on a proof of concept (www.webme.co.nz/poc) for some site components produced using CSS.

As I have pretty much stretched beyond my knowledge and experience on this, I’d appreciate some people more experienced at CSS having a look at what I have done and offering advice on how it could be improved.

Things in particular that I am interested in are:

  1. Ways in which the CSS could be better optimized for greater efficiency.
  2. Any bugs in specific browsers and how they may be fixed (I have only tested in Chrome 5, FF3.5, IE7) however, as it will be used in an administrative application for a private company, older or more obscure browsers are unlikely to be used. IE6 may be used though.
  3. Anything I have overlooked that may cause issues for some users (I have not spent much time considering accessibility issues, so that is one area I would be interested in receiving advice on)
  4. How to line up the left/right images with the numbers in the pagination.
  5. How to vertically align the form labels so they are level with the middle of the fields in IE7
  6. Any other comments (please be gentle ;o) )

The Components

  1. Fixed header with flexible main content section. I based this on the solution described in Test your CSS skills Quiz 1.

  2. Multi-level drop menu (see “Tools” for third level). Based on a solution I picked up some time ago. I’m not sure that display:none is the best way to hide the drop-downs, so would like to hear comments on that.

  3. Scrolling table with fixed headers. Another one I picked up from the Test your CSS skills series, this time Quiz 22

  4. Tabbed menu. This is simply a formatted unordered list, but I’d be interested in hearing a method of fixing the 1px gap where it meets the main panel.

Thanks for your interest.

2

Hi, while this mmay be a CSS related question. I’d ask any design/accessibilty/any other questions not related to cross browser issues in a different part of hte forum. I’ll ask this to be moved. I’ll answer some questions while I’m still typing.

Dropdowns should be hidden via a huge left margin (margin-left:-999em) and hten on hover bring it back to 0 (margin-left:0)
Coordinates shouolld be used as always (you don’t seem like a beginner)

3

Thanks for responding, Ryan

I apologize for mixing question types. My main interest is in producing good quality, efficient CSS, which is why I posted it here.

I’m not quite sure what you mean by “any design/accessibilty/any other questions not related to cross browser issues”. Is the CSS forum only for posts related to cross-browser issues?

Yes, I believe I read that elsewhere. Thanks for the confirmation.

Thank you for the compliment. I am not a beginner, but also not an expert by any means. However, I am reasonably adept at adapting the work of others for my needs. I guess you could say I am experienced enough to know where I lack expertise :wink: which is why I appreciate peer review of my work.

4

Hi,
You have posted in the right forum as your questions are CSS related.

  1. How to line up the left/right images with the numbers in the pagination.
    As for vertical alignment of the arrow images the vertical-align needs to be set on the inline elements and not the p tag itself. Vertical-align only works on inline elements (including inline-block) and tables.

Your images have about 2px worth of whitespace at the top of them which throws the true alignment off slightly.

Just set vertical-align:midlle; on the image and remove vertical-align:bottom; from the anchor.

#tabSection p.pagination {
    border-top: 1px solid #91a7b4;
    clear: both;
    padding-top: 6px;
    text-align: center;
    [COLOR=Red]/*vertical-align: bottom;*/[/COLOR]
}
#tabSection p.pagination a {
    text-decoration: none;
    [COLOR=Red]/*vertical-align: bottom;*/[/COLOR]
}
#tabSection p.pagination [B]img[/B] {
   [COLOR=Blue] vertical-align:middle;[/COLOR]
    [COLOR=Blue]background:cyan;[/COLOR][COLOR=DarkGreen]/*test BG color*/[/COLOR]
    [COLOR=Blue]padding-bottom:2px;[/COLOR][COLOR=DarkGreen]/*compensate image offset*/[/COLOR]
}
5

Aha! That’s where I was thrown off. I was working on the supposition that vertical-align operated on the contents of the element (like text-align) whereas it operates on the element itself. No wonder I have had so much trouble getting it to work :wink:

Perfect! Thanks for your help.

6

This guided me to a solution for aligning the form labels as well.

Thanks again!

7

I expected that you might find the solution for the labels after I explained how v-a works with inline elements. :slight_smile:

8

Thanks again for the tip. I’ve managed to put this into effect.

9
  1. Tabbed menu. This is simply a formatted unordered list, but I’d be interested in hearing a method of fixing the 1px gap where it meets the main panel.

You should be able to fix that gap with bottom padding and a negative margin to match. I was able to eliminate the bottom border by using 2px, you might want to set that active class on another list item and see how it works there.

#tabSection ul.tabs li.active {
    background-color: #f0ecde;
    background-image: url("tab-left-active.gif");
    [COLOR=Red]/*border-bottom: 1px solid #f0ecde;*/[/COLOR]
[COLOR=Blue]    padding-bottom:2px;
    margin-bottom:-2px[/COLOR]
}
10

Fantastic! That works great.

I’d tried using a negative margin on the .inner div which closed the gap, but also covered the bottom-border on the .tabs ul

Thanks for taking the time to help me figure these things out. I really appreciate it.

Your solution gave me some ideas about the gap at the other end too and I managed to solve that as well! Woohoo!

The issue I has was that the bottom border on the .tabs ul ended 2px short of the right hand corner. Adding a 1px border each side of the ul fixed the alignment, but I didn’t want borders on the ends. But that got me thinking - how else can I add those 2px? Adding 2px padding-left to the ul fixed the bottom-border issue, but left the first tab 2px to the right. So I gave the first tab a class of “first” and applied a -2px margin-left to it. Solved!

It even works in IE7 :smiley:

11

Oh, by the way, both our fixes work great no matter which tab is active.

12

What Ryan meant was that we have a specific forum for Reviews and as you titled your thread “Code Review Request” then it could be perceived that you were looking for general reviews on how the sites works, looks and feels. Those types of questions must be asked in the review forum so that we don’t have people posting “does my site look ok” type of questions and cluttering up the forums :slight_smile:

However, questions about bugs and fixes and css problems are fine here:) It’s just the review type of questions that you shold avoid but any other fixes you need looked at then fire away;)

Usually it’s better to ask one or two questions at a time as a whole list frightens people away :slight_smile:

13

Thanks for the clarification, Paul. I see your point. I guess I did throw quite a bit into the pot at once.

14

Hi Paul

I just checked out the review forums you linked to and they all seem to be focused on design, layout, usability, etc. In this post, I was not after a critique of my design or content. I was after a review of the CSS I used to create it.

I accept that I may have been unclear and it may have appeared that I was after a review of the design. I also accept that I put too many questions in one post. I apologize.

My primary interests when I started this thread were:

  1. have a more experienced CSSer review the CSS and let me know if it could be improved and/or if there is anything that might break in a certain browser (most significantly IE6). Things like “____ will break in IE6 so you should add ___ (hack) to prevent that” or “If you change ____, you can reduce your CSS file by X lines” or, for a specific example, “To hide the drop menus, it is better to use negative margin than display:none.”

  2. receive advice on the specific issues I had (i.e. vertical alignments, gaps in tab menu, etc)

Thank you again to those who helped.

15

If you want to support IE6 then you need to provide additional rules for any of the following.

input[type=“submit”]

ul.nav li:hover>a

IE6 doesn’t understand attribute selectors or the child selector so you would need to make alternative arrangements using a class or similar.

This code won’t work:


* html #content {
    background: #fff;
    height: 100%;
   [B] margin:20px;[/B]
    overflow: scroll;
  [B]  padding-bottom: 20px;
    padding-top: 20px;[/B]
    position: relative;
    z-index: 100;
}

height:100% + 40px margin + 40px padding = much to big.

You have the double margin float bug in IE6. Add display:inline to floats that have margins and are first, last or only floats in a row.


#tabSection .panel {
    background: #f0ecde;
    border-bottom: 1px solid #91a7b4;
    border-left: 1px solid #91a7b4;
    border-right: 1px solid #91a7b4;
    float: left;
    height: 300px; /*must be same as in #tabSection .panel .inner */
    margin: 0px 0 0 2%; /*must be same as in #tabSection ul.tabs */
    overflow: hidden;
    padding: 57px 0 30px 0; /*sets space above and below .inner div */
    position: relative;
    width: 95%; /*must be same as in #tabSection ul.tabs */
[B]    display:inline;[/B]
}

I assume this is taken from one of my fixed header demos but you don’t seem to be utilising it correctly (although I know you haven’t finished yet).


* html body {height: 100%; margin: 0; [B]padding: 180px 0 20px 0}[/B]

The padding for IE6 is to make room for fixed header and footer elements so needs to match whatever your requirements are.

If an element has negative margins in iE6 it will most likely need position:relative to show the negative portion.

It is needed here along width display:inline for the double margin bug.


#tabSection ul.tabs li.first {
    margin-left: -2px;
 [B]   position:relative;
    display:inline;[/B]
}

You will find your tabs will jog in and out by 1px in ie6 due to rounding errors and the percentage margins used.

The header needs to be placed absolutely at the top for ie6.


* html body {height: 100%; margin: 0[B]; padding: 45px 0 0px 0[/B]}
* html #header{[B]width:100%;position:absolute;top:0;left:0[/B]}

IE6 needs a lot more work but I’ve run out of time today :slight_smile:

16

Paul, thank you very much for taking the time you have. I really appreciate your advice.

Looking at the IE6 fixes you have already given and noting that you say even more needs to be done helps me understand why so many people hate IE6 :wink:

I will check with my employer as to whether we actually need to support IE6. As it would only be used in an administrative backend for a private company, we will probably have some control over what browsers are used.

I appreciate your time and don’t want you to waste it finding further IE6 solutions for this if we don’t need to support it, so unless you think it would be a worthwhile exercise in order to help others learn IE6 fixes, please don’t bother spending any more time on it.

17

No Problem - just shout if you do need to support IE6 and we’ll go through the fixes needed :slight_smile: