patternjavascriptMinor
Start of a responsive website for a legal firm
Viewed 0 times
websitefirmresponsivelegalforstart
Problem
Please have a look at the HTML/CSS/JS (there is very little JS at the moment) and give me some feedback as to how my code looks to you, what level of skills it looks like I have and how I can improve. It's not anywhere near being completed but it should give you a good idea of where I'm at and where I'm going with it.
`h1 {
font-size: 2em;
}
h2 {
font-size: 1.5em;
}
h3 {
font-size: 1.25em;
}
#header-main {
background: #fff;
padding: 0;
}
#header-main .logo h1 {
font-size: 1.5em;
font-weight: bold;
color: #60b8f4;
text-transform: uppercase;
margin-bottom: 10px;
}
#header-main .phone {
text-align: left;
font-weight: bold;
margin-bottom: 10px;
}
#header-main .mobile-menu-wrapper {
text-align: right;
}
#header-main .mobile-menu-wrapper .mobile-menu-title {
font-size: 1.3em;
text-transform: uppercase;
margin-right: 10px;
}
@media (min-width: 768px) {
#header-main .phone {
text-align: right;
};
}
footer {
background: #363b3d;
min-height: 100px;
color: #ddd;
font-size: 0.85em;
}
footer .social-wrapper:after {
content: " ";
display: block;
clear: both;
}
footer .social-wrapper
// Get user's country. May be useful in the future.
user_country = '';
$.get("http://ipinfo.io", function(response) {
user_country = response.country;
}, "jsonp");
$( document ).ready(function() {
$('.mobile-menu').click(function(){
$('#mainnav .menu').slideToggle("slow", function(){
});
});
});
$(window).load(function() {
// when column wraps to next row, set padding-left to 0
$('#main-content .row').each(function(index){
col_offset_first = $(this).find('.col:first').offset().left;
$(this).find('.col').each(function(index){
col_offset = $(this).offset().left;
if(col_offset == col_offset_first && index > 0) {
$(this).css('padding-left', 0);
}
});
});
});
`h1 {
font-size: 2em;
}
h2 {
font-size: 1.5em;
}
h3 {
font-size: 1.25em;
}
#header-main {
background: #fff;
padding: 0;
}
#header-main .logo h1 {
font-size: 1.5em;
font-weight: bold;
color: #60b8f4;
text-transform: uppercase;
margin-bottom: 10px;
}
#header-main .phone {
text-align: left;
font-weight: bold;
margin-bottom: 10px;
}
#header-main .mobile-menu-wrapper {
text-align: right;
}
#header-main .mobile-menu-wrapper .mobile-menu-title {
font-size: 1.3em;
text-transform: uppercase;
margin-right: 10px;
}
@media (min-width: 768px) {
#header-main .phone {
text-align: right;
};
}
footer {
background: #363b3d;
min-height: 100px;
color: #ddd;
font-size: 0.85em;
}
footer .social-wrapper:after {
content: " ";
display: block;
clear: both;
}
footer .social-wrapper
Solution
Whitespace in expressions
There is a distinct lack of whitespace in all of your expressions in Sass (eg.
Underscore optional
When you import a file, the underscore is completely optional, just like the extension (unless you happen to have both
Is the same as
Font-size
There's no good reason to have all of these font declarations:
It can be shortened to this:
Note the lack of unit on the line-height. This has been the standard practice for nearly 10 years (see: http://meyerweb.com/eric/thoughts/2006/02/08/unitless-line-heights/)
Unsemantic class names
Naming a class after an existing HTML element should raise a red flag. What makes an element strong? Is it a stern warning? Is it a call-to-action element? Can it bench press 300lbs? What?
Same goes for "action-green". Will it still make sense when the color scheme doesn't include green anymore?
Unsemantic markup
Now I see what the strong class is for.
Markup should be chosen based how well it fits the content semantically, not because it makes styling a little easier. This looks almost the same and has better markup semantically:
SCSS:
This probably doesn't do what you think it does...
In a number of your media queries, you're setting the variable
Reinventing the wheel
There are a few libraries out there that provide mixins for handling properties that require prefixes. Personally, I use Compass. The ones it provides are quite a lot more robust than the ones you're using (their gradient mixin will generate an SVG for IE9 as well as the old style webkit gradient, which is still relevant for older mobile browsers).
Is that JavaScript doing what I think it's doing?!
JavaScript isn't my thing, and I certainly don't use jQuery, but it looks like you're using it to compensate for adding margins on the left side of all of your elements that are the first element on that row, yes? You don't have to do that!
Use negative margins on your container element instead:
Orphans
This doesn't appear to be used:
Duplicate markup for mobile and desktop
This doesn't seem very fleshed out at the moment, but serving duplicate content with the intent of hiding one and styling the other depending on if it is a "mobile" device vs. a "desktop" device is not a path I recommend following. Your page should still make sense even when there's no styling at all. Reading through the same set of menu items twice would be very confusing for the poor guy who is stuck using Lynx to view your page.
There is a distinct lack of whitespace in all of your expressions in Sass (eg.
$colwidth: $i/12*$totwidth). This has been known to cause confusion with the parser in certain versions of Sass (notably with subtraction). I would recommend always including whitespace around every arithmetic operator, rather than just the ones that can cause problems.Underscore optional
When you import a file, the underscore is completely optional, just like the extension (unless you happen to have both
foo.scss and _foo.scss).@import "_functions";Is the same as
@import "functions";Font-size
There's no good reason to have all of these font declarations:
body {
font-family: $main-font;
font-size: $base-font-size;
}
p {
line-height: 1.5em;
font-size: 1em;
}It can be shortened to this:
body {
font: $base-font-size/#{1.5} $main-font;
}Note the lack of unit on the line-height. This has been the standard practice for nearly 10 years (see: http://meyerweb.com/eric/thoughts/2006/02/08/unitless-line-heights/)
Unsemantic class names
Naming a class after an existing HTML element should raise a red flag. What makes an element strong? Is it a stern warning? Is it a call-to-action element? Can it bench press 300lbs? What?
.strong {
font-weight: bold;
}Same goes for "action-green". Will it still make sense when the color scheme doesn't include green anymore?
Unsemantic markup
Now I see what the strong class is for.
Headline 1 - Lorem ipsum dolor sit amet, graecis rationibus quo ad, no essent eligendi voluptua cum. Vel dolores commune et, te harum dicam voluptatibus vis.Markup should be chosen based how well it fits the content semantically, not because it makes styling a little easier. This looks almost the same and has better markup semantically:
Headline 1
Lorem ipsum dolor sit amet, graecis rationibus quo ad, no essent eligendi voluptua cum. Vel dolores commune et, te harum dicam voluptatibus vis.
Headline 1
Lorem ipsum dolor sit amet, graecis rationibus quo ad, no essent eligendi voluptua cum. Vel dolores commune et, te harum dicam voluptatibus vis.SCSS:
h3 { // add class here if appropriate
display: inline;
font-size: inherit;
&:after {
content: ' - ';
}
+ p {
display: inline;
&:after {
display: table;
content: '';
margin-top: 1em;
}
}
}This probably doesn't do what you think it does...
In a number of your media queries, you're setting the variable
$media-size equal to whatever the min-width value is of that media query. Once the closing curly brace is in place, that variable is lost. I don't even know how this code compiles unless the variable is declared in some other block of code that you didn't provide.@media (min-width: 992px) {
$media-size: 992;
}
.foo {
bar: $media-size; // Undefined variable: "$media-size".
}Reinventing the wheel
There are a few libraries out there that provide mixins for handling properties that require prefixes. Personally, I use Compass. The ones it provides are quite a lot more robust than the ones you're using (their gradient mixin will generate an SVG for IE9 as well as the old style webkit gradient, which is still relevant for older mobile browsers).
Is that JavaScript doing what I think it's doing?!
JavaScript isn't my thing, and I certainly don't use jQuery, but it looks like you're using it to compensate for adding margins on the left side of all of your elements that are the first element on that row, yes? You don't have to do that!
Use negative margins on your container element instead:
.outer {
background: yellow;
margin-left: -5%; // negative of the margin you're going to add
overflow: hidden;
.inner {
width: 20%;
background: rgba(blue, .3);
margin-left: 5%;
float: left;
}
}Orphans
This doesn't appear to be used:
@function padding-width() {
@return 1.5;
}Duplicate markup for mobile and desktop
This doesn't seem very fleshed out at the moment, but serving duplicate content with the intent of hiding one and styling the other depending on if it is a "mobile" device vs. a "desktop" device is not a path I recommend following. Your page should still make sense even when there's no styling at all. Reading through the same set of menu items twice would be very confusing for the poor guy who is stuck using Lynx to view your page.
Code Snippets
@import "_functions";@import "functions";body {
font-family: $main-font;
font-size: $base-font-size;
}
p {
line-height: 1.5em;
font-size: 1em;
}body {
font: $base-font-size/#{1.5} $main-font;
}.strong {
font-weight: bold;
}Context
StackExchange Code Review Q#96374, answer score: 2
Revisions (0)
No revisions yet.