Calendar in HTML + CSS + JS

As I'm new to HTML + CSS + JS I've been building a calendar (which I think anyone can use as a basis) but would like to get your suggestions on how I could improve my code specifically from the perspectives of:

• Adherence to best practices in HTML + CSS + JS.

• Usability.

• Browser compatibility.

Note: The console logging in the JavaScript is only for debugging, not the final version.



`var Calendar = function(o) {
//Store div id
this.divId = o.ParentID;

// Days of week, starting on Sunday
this.DaysOfWeek = o.DaysOfWeek;

console.log("this.DaysOfWeek == ", this.DaysOfWeek)

// Months, stating on January
this.Months = o.Months;

console.log("this.Months == ", this.Months)

// Set the current month, year
var d = new Date();

console.log("d == ", d)

this.CurrentMonth = d.getMonth();

console.log("this.CurrentMonth == ", this.CurrentMonth);

this.CurrentYear = d.getFullYear();

console.log("this.CurrentYear == ", this.CurrentYear);

var f=o.Format;

console.log("o == ", o);

console.log("f == ", f);

//this.f = typeof(f) == 'string' ? f.charAt(0).toUpperCase() : 'M';

if(typeof(f) == 'string') {
this.f = f.charAt(0).toUpperCase();
} else {
this.f = 'M';
}

console.log("this.f == ", this.f);
};

// Goes to next month
Calendar.prototype.nextMonth = function() {
console.log("Calendar.prototype.nextMonth = function() {");

if ( this.CurrentMonth == 11 ) {
console.log("this.CurrentMonth == ", this.CurrentMonth);

this.CurrentMonth = 0;

console.log("this.CurrentMonth == ", this.CurrentMonth);

console.log("this.CurrentYear == ", this.CurrentYear);

this.CurrentYear = this.CurrentYear + 1;

console.log("this.CurrentYear == ", this.CurrentYear);
} else {
console.log("this.CurrentMonth == ", this.CurrentMonth);

this.CurrentMonth = this.CurrentMonth + 1;

console.log("this.CurrentMonth + 1 == ", this.CurrentMonth);
}

this.showCurrent();
};

// Goes to previous month

HTML

The HTML looks mostly fine. Just a few minor stylistic comments:

• Variable names: You use dromedaryCase for some ids, and lowercasenames for others. It’s nice to be consistent. Also, don’t prefix variable names with “div”; it’s redundant info.

• Comments: It’s not obvious to anybody reading the HTML what the point of the divcalendartable div is for, as it appears to be empty. You should add some comments explaining that you’re using JavaScript to populate this div. (I think you should try to do more of the rendering in HTML and just embellish it with JS, but doing so probably requires rethinking the entire calendar.)

• No JS: this will look very kooky in a browser that doesn’t have JS enabled. It’s enough to wrap a message in a ` tag to explain that you need JS enabled for it to work; you just have to do something so that it fails gracefully.

I’ll come back to the HTML when I comment on the JS.

CSS

You can make some improvements here:

-
Comments: Your CSS needs more comments. (This is not just you: most CSS has very sparse commenting.) In particular, I’d suggest comments that:

• Organise your CSS into the different parts of the calendar. e.g. a section for the base table definitions, the buttons, the calendar cells. If you want to change a specific part of the app, this makes it much easier to find the exact style definition you want.

• Explain why you chose particular CSS rules. In particular, anything that’s non-obvious or was tricky to pin down – this will save you loads of time when you come to debug this later.

-
Consolidate: You have lots of selectors with very similar attributes that differ in only a few ways. I’d put multiple selectors on the common rules, with distinct selectors for the few rules where they differ. Not only is this more compact, but it highlights the differences between them.

For example, for the final pair of rules, write it as:

.swim, .chrono {
font-family: Arial, Helvetica, sans-serif;
font-size: 80%;
text-align: center;
color: #F5F5F5;
margin-bottom: 5px;
padding: 5px;
}

.swim {
background: #445511;
}

.chrono {
background: #778899;
}


JS

This is by no means a comprehensive review, but I’ve listed a variety of comments below. But first, two general comments:

• Use better variable names. Single-letter variable names make it really hard to understand what a variable represents, or to follow it through the script. (And it’s a pain to grep for.) Replace all your variable names with something that describes what that variable should store.

• Write more comments. Although the exact code is fairly easy to follow, you could definitely explain what you’re doing a bit more. (And to be clear: explain the why of what the code does, not what the code does.)

Okay, onto some comments on the individual comments:

-
Calendar. You should tell us what input this function takes, in particular what attributes the function expects it to have (and what attributes this will have), and what those attributes correspond to.

-
nextMonth. This looks fine, but here’s a way you can slightly tidy up the CurrentMonth iterator:

this.CurrentMonth = (this.currentMonth + 1) % 12;

and then you can remove the else branch from your if statement.

I would replace incrementing the month directly with a call to nextYear. Although nextYear is a very similar function, it helps to make the intent of the code clear.

So here’s how I’d rewrite it:

Calendar.prototype.previousMonth = function() {
this.CurrentMonth = (this.CurrentMonth + 12) % 12;

// If we have wrapped around to the first month of the next
// year, then we need to update the year attribute appropriately.
if (this.CurrentMonth == 0) {
this.nextYear();
}

this.showCurrent();
}


You can obviously do similar for previousMonth. (You may need to move the nextYear and previousYear definitions before these functions.)

-
previousYear. You can tidy up the year increments to make them shorter and easier to read:

this.CurrentYear--;


-
showCurrent. No comments.

-
Calendar.prototype.Calendar. More descriptive input variables.

At the top of the function, you have a pair of ternary operators. Although that’s not bad per se, since they both have the same condition, I think it’s clearer to write them as an if … else block:

if (typeof(y) == 'number') {
this.currentYear = y;
this.currentMonth = m;
} else {
this.currentYear = null;
this.currentMonth = null;
}


The ternary operators starting var lastDateOfLastMonth and var p = dm are more complicated and should definitely be broken up. You should also add some comments to explain whatever it is they’re doing, particularly the magic numbers (1, 0, -5 and 2) on the latter.

The z0* loop variables make these loops very hard to follow.

-
window.onload. See previous comments about using better variable names.

(Also, I suspect that if you initialise your var c = new Calendar()` before the HTML, then you c

StackExchange Code Review Q#85254, answer score: 10