HiveBrain v1.2.0
Get Started
← Back to all entries
patterncssMinor

CSS Good Practice on a JavaScript Plugin

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
plugincodereviewcssstackoverflow
csspracticejavascriptplugingood

Problem

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;

Solution

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 ` for with less work for example. Inversely, if all '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.


These target different elements

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


See in this situation putting a .submenu class on the inner 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 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 .

-
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 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 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.

This would be an ideal situation to use classes, the elements th

Code Snippets

/* Bad */
div.paginateContainer > ul > li.paginateActive > a

/* Good*/
.paginateContainer .paginateActive a
<ul class="menu">
    <li>
        <ul>
            <li></li>
        </ul>
    </li>
</ul>
.menu > li { /* ... */}
.menu li { }
.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;
}

Context

StackExchange Code Review Q#23680, answer score: 4

Revisions (0)

No revisions yet.