6
\$\begingroup\$

I've realized a jQuery Plugin recently and made a basic default CSS for it.

As I'm mainly a server-side guy, I'm not too familiar with CSS and would like to have insight about what could be improved.

The main issues I've targeted are :

  • Are the CSS selectors appropriate, knowing that this CSS is meant to be on a plugin and to be side-effect-less as much as possible.
  • Are the property usages appropriate or are there better way of using some of the properties I have used.

Any suggestion of code style is also highly appreciated!

Here is a working fiddle

Here is the css :

div.paginateContainer {
    display : block;
    text-align: center;
}

div.paginateContainer > ul.paginateList{
    display: inline-block;
}

div.paginateContainer > ul > li{
    display: inherit;
}

div.paginateContainer > ul > li > a.paginateAnchor{
    font-family: "Helvetica Neue",Helvetica,Arial,sans-serif;
    text-decoration: none;
    cursor: pointer;

    line-height: 20px;
    padding: 4px 12px;
    background-color: #ddd;
    color: #08c;
    font-size : 16px;
    min-width: 16px;
    float: left;

    border-top: 1px solid #ccc;
    border-bottom: 1px solid #ccc;

    /* Select None */
    -webkit-touch-callout: none;
    -webkit-user-select: none;
    -khtml-user-select: none;
    -moz-user-select: none;
    -ms-user-select: none;
    user-select: none;
}

div.paginateContainer > ul > li > a.paginateAnchor:hover {
    color : #fff;
    background-color: #2ae;
    border-top: 1px solid #2ae;
    border-bottom: 1px solid #2ae;
}

div.paginateContainer > ul:first-child > li.paginatePrevious > a{
    border-top-left-radius: 10px;
    border-bottom-left-radius: 10px;
    border: 1px solid #ccc;
    font-size : 18px;
    font-weight: bold;
}

div.paginateContainer > ul:first-child > li.paginatePrevious > a:hover {
    border: 1px solid #2ae;
}

div.paginateContainer > ul:first-child > li.paginateNext > a {
    border-top-right-radius: 10px;
    border-bottom-right-radius: 10px;
    border: 1px solid #ccc;
    font-size : 18px;
    font-weight: bold;
}

div.paginateContainer > ul:first-child > li.paginateNext > a:hover {
    border: 1px solid #2ae;
}

div.paginateContainer > ul:last-child > li > a {
    border-radius: 10px;
    font-size: 18px;
    font-weight: bold;
}

div.paginateContainer ul.paginateList:last-child {
    float: right;
}

div.paginateContainer > ul > li.paginateActive > a,
div.paginateContainer > ul > li.paginateActive > a:hover {
    color : #fff;
    cursor: default;
    background-color: #08c;
    border-top: 1px solid #08c;
    border-bottom: 1px solid #08c;
}

div.paginateContainer > ul:first-child > li.paginateDisabled > a, 
div.paginateContainer > ul:first-child > li.paginateDisabled > a:hover {
    color : #bbb;
    background-color: #eee;
    cursor: default;
    border: 1px solid #ddd;
}

Note that I've posted a question to review the JavaScript also, so this post is only about CSS. The JavaScript having no real dependency with the CSS in this case.

\$\endgroup\$
4
\$\begingroup\$

You styles are pretty solid, it's mainly your selectors that have issues I feel. Here are some notes:

  • It's normally best to not include the type of element if it has a class. This is for maintainability and extensibility purposes, you could switch in <ol> for <ul> with less work for example. Inversely, if all <ul>'s have the same style, they don't even need the same class.

  • You never want to go too deep with you CSS selectors, more specific is less flexible.

    /* Bad */
    div.paginateContainer > ul > li.paginateActive > a
    
    /* Good*/
    .paginateContainer .paginateActive a
    
  • Avoid the use of the direct descendant > selector where ever possible, again it just locks your CSS down and makes it less flexible. I recommend only ever using this if you need it to add styles to a legacy application or have a situation where you have a hierarchy something like this that doesn't make sense to add different classes.

    <ul class="menu">
        <li>
            <ul>
                <li></li>
            </ul>
        </li>
    </ul>
    

    These target different elements

    .menu > li { /* ... */}
    .menu li { }
    

    See in this situation putting a .submenu class on the inner <ul> won't make a different, .menu li will still target both.

  • Try to keep your selectors consistent, for example you refer to both ul.paginateList and ul to mean the same thing.

  • I would add a different class to your first <ul> so you don't need to refer to it as ul:first-child, you may want to change the order in the future for example. Maybe only include .paginateList on the first <ul>.

  • Due to your use of specific pixel font sizes your plugin may require modification in order to be used on someone's site. Generally these days we use 'ems' which represent the size of the character 'M'. Currently your font-size is fixed at 18px and it will not change whatever site you put it on, using font-size:1.2em for example would scale to be 20% larger than the font-size of the element containing .paginateContainer.

    It also allows for better mobile support, taking advantage of the default font size set by the user. This does require a bit of work though as you should apply it to padding, border-radius, line-height, font-size and min-width (everything except border-size).

And here is your modified CSS

.paginateContainer {
    display : block;
    text-align: center;
}


.paginateContainer ul {
    display: inline-block;
}

.paginateContainer li {
    /* Better to state the display explicitly */
    /*display: inherit;*/
    display:inline-block;
}

.paginateContainer a {
    font-family: "Helvetica Neue",Helvetica,Arial,sans-serif;
    text-decoration: none;

    /* Put the cursor rule on :hover */
    /*cursor: pointer;*/

    line-height: 20px;
    padding: 4px 12px;
    background-color: #ddd;
    color: #08c;
    font-size : 16px;
    min-width: 16px;
    float: left;

    border-top: 1px solid #ccc;
    border-bottom: 1px solid #ccc;

    /* Select None */
    -webkit-touch-callout: none;
    -webkit-user-select: none;
    -khtml-user-select: none;
    -moz-user-select: none;
    -ms-user-select: none;
    user-select: none;
}

.paginateContainer a:hover {
    color : #fff;
    background-color: #2ae;
    border-top: 1px solid #2ae;
    border-bottom: 1px solid #2ae;
    cursor:pointer;
}

.paginateContainer ul:first-child .paginatePrevious a {
    border-top-left-radius: 10px;
    border-bottom-left-radius: 10px;
    border: 1px solid #ccc;
    font-size : 18px;
    font-weight: bold;
}

.paginateContainer ul:first-child .paginatePrevious a:hover {
    border: 1px solid #2ae;
}

.paginateContainer ul:first-child .paginateNext a {
    border-top-right-radius: 10px;
    border-bottom-right-radius: 10px;
    border: 1px solid #ccc;
    font-size : 18px;
    font-weight: bold;
}

.paginateContainer ul:first-child .paginateNext > a:hover {
    border: 1px solid #2ae;
}

.paginateContainer ul:last-child a {
    border-radius: 10px;
    font-size: 18px;
    font-weight: bold;
}

.paginateContainer ul:last-child {
    float: right;
}

.paginateContainer .paginateActive a,
.paginateContainer .paginateActive a:hover {
    color : #fff;
    cursor: default;
    background-color: #08c;
    border-top: 1px solid #08c;
    border-bottom: 1px solid #08c;
}

.paginateContainer ul:first-child .paginateDisabled a, 
.paginateContainer ul:first-child .paginateDisabled a:hover {
    color : #bbb;
    background-color: #eee;
    cursor: default;
    border: 1px solid #ddd;
}

EDIT: As for you question about why I've used <a> tags instead of the class. It's a similar situation to why I've referred to your lists as ul and not .paginateList, the class literally provides no additional information because every single <a> tag has the class present. Changing it to use a instead of .paginateAnchor will make both your HTML and CSS more lightweight.

I also noticed just now that you're using inline styles applied with your JavaScript.

Inline styles

This would be an ideal situation to use classes, the elements that are inline-block can have no class (which will use div.paginateContainer > ul > li { display:inherit; }) and the hidden elements can have a class called .hidden. The reasoning for this is separation of concerns, your JavaScript should not need to do any styling, instead it should offload that detail to your CSS so everything is in one place.

\$\endgroup\$
  • \$\begingroup\$ First, thank you for your answer it's really insightful! I have a question. You talk about using classes instead of elements. You do that for everything except the anchor elements. Is there a reason why specifically for this element? Also would you mind finishing the sentence "I recommend only ever using this if you have" You seem to bring an important point but you forgot to end it!! I'll let a few day pass and will mark it as accepted afterwards \$\endgroup\$ – Hugo Dozois Mar 10 '13 at 15:54
  • \$\begingroup\$ @HugoDozois I was jumped around a bit when I wrote it, I guess I missed that part :) updated the answer to talk about direct descendant, a vs class and styles applied with js. \$\endgroup\$ – Daniel Imms Mar 11 '13 at 1:39
  • \$\begingroup\$ About the .foo > .bar selector - it is beneficial in terms of performance - broswers will look for .foo only on .bar parent instead of navigating the whole DOM tree to the root. Also specificity makes your css more solid - instead of losing flexibility (with all due exceptions of course - e.g. over-qualified selectors) \$\endgroup\$ – Luca Mar 11 '13 at 1:45
  • \$\begingroup\$ @Luca I actually mentioned that in a post on StackOverflow and got downvoted a bunch. While it is true, you'll find that most of the CSS community is against it because it prevents you from thinking in a modular and reusable way. The performance of CSS is pretty insignificant anyhow unless the page is gigantic or you're using CSS3 animations/transitions/transforms/etc. \$\endgroup\$ – Daniel Imms Mar 11 '13 at 1:50
  • \$\begingroup\$ @Tyriar the inline styles are caused by show() hide() methods. While I agree I could have used an hidden class, the code is clearer using show and hide and is independent of the css style sheet. While as if there is an hidden class css needed, the user has to include my style sheet. \$\endgroup\$ – Hugo Dozois Mar 11 '13 at 1:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.