Skip to content

Scotland class 5 - karma clone #227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bonboh wants to merge 4 commits into from
Closed

Scotland class 5 - karma clone #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ef904d
Update index.html
M-hayek Jun 6, 2021
0d96f3e
navigation bar and into
M-hayek Jun 7, 2021
c7e8dc3
Complete page
M-hayek Jun 7, 2021
0018dc0
final
M-hayek Jun 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 106 additions & 1 deletion css/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,16 @@

body {
font-family: 'Roboto', sans-serif;
font-size: 16px;
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
font-size: 16px;
font-size: 1rem;

We should use relative units like rem, especially for font-size. If we use pixels, then if a user changes their browser default font size setting then the screen doesn't increase in size

-webkit-font-smoothing: antialiased;
}
width:100%;
height:100%;
Comment on lines +8 to +9
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
width:100%;
height:100%;

We don't need these properties so we can remove them

color:#838994;
text-align: center;
line-height: 25px;
box-sizing: border-box; /*padding included in the box size*/
display: block; /*each element as a block*/
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
display: block; /*each element as a block*/

Normally, it's not a good idea to turn every element into a block, because when we use inline elements like <a> and <strong> for example, they will all be on separate lines which does not make sense

}

/**
* Add your custom styles below
Expand All @@ -16,4 +24,101 @@ body {
* - When using Flexbox, remember the items you want to move around need to be inside a parent container set to 'display: flex'
*/

#nav-bar {position:fixed;
top:0;
height: 70px;
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
height: 70px;
height: 4.5rem;

We should use relative units where possible. Same applies to all the other uses of px in this file

width:100%;
right:40px;
background-color: white;
}

#nav-bar ul {list-style-type:none;
display:flex;
justify-content: flex-end;
}

#nav-bar ul a:hover {color:#F15B2A;
}
Comment on lines +40 to +41
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#nav-bar ul a:hover {color:#F15B2A;
}
#nav-bar ul a:focus,
#nav-bar ul a:hover {
color:#F15B2A;
}

We should also add :focus so that keyboard users who use the Tab key to jump to the link also get a change in colour.

Also, for coding the formatting of the code matters in software teams. Typically, it's structured like in my suggestion. This applies to the whole file


#nav-bar img {height:60px;
width:200px; position: absolute;
left: 10px;
top: 10px;
z-index: -1;
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
z-index: -1;

z-index is not needed here so we can remove it

}

.nav-link {text-decoration:none;
color:darkgray;
font-weight:bolder;
display:block;
padding:8px;
}

.nav li:first-child > a {color: rgb(46, 44, 44);}


article {margin-top:70px;}


h1 {color: white; font-weight:lighter; font-size: 2em;}
h2 {color: white; font-weight:lighter;}
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
h2 {color: white; font-weight:lighter;}
#intro h2 {color: white; font-weight:lighter;}

We want the h2 to only be white in the #intro section, so we can update the selector to only apply to the h2s in the intro


#intro {background-image: url('https://raw.githubusercontent.com/willem-blauenberg/portfolio/master/projects/karma/img/first-background.jpg');
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#intro {background-image: url('https://raw.githubusercontent.com/willem-blauenberg/portfolio/master/projects/karma/img/first-background.jpg');
#intro {background-image: url('/img/first-background.jpg');

There's an image in the codebase that we can use, which is better than linking to an external image

height: 700px;
width:100%;
background-size: cover left;
background-position: center;
padding-top:50px;
text-align: center;
background-repeat: no-repeat;

display: flex;
justify-content: center;
align-items: center;
}

#into-content {}
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#into-content {}

As this rule is empty, we should delete it because software teams don't leave empty code in the codebase


#learnbutton { background-color: #F15B2A;
border: none;
color: white;
padding: 15px 32px;
text-align: center;
text-decoration: none;
display: inline-block;
}

#content {height:350px}

#content h1 {color: rgb(46, 44, 44); }

.service-img {width:100px; height:100px; }

.services {width:200px;
text-align:center;
display:inline-block;
float:center;
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
float:center;

float doesn't apply here so we can remove it

padding:10px;
}

.servicename {text-decoration: none; color:rgb(46, 44, 44);}


footer {height:220;
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
footer {height:220;
footer {height:220px;

It looks like we're missing units here. Also, we should be using relative units like rem as mentioned in my other comment

text-align:center;
margin:auto; /* horizontally center the element within its container*/
max-width: 60%;
border-top: 1px solid lightgrey;}

footer ul {margin:auto;}

footer li {padding:10px;
display:inline-block;
}

footer ul li img { width: 30px;
}

footer ul li img : hover {cursor:pointer;
}
Comment on lines +122 to +123
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
footer ul li img : hover {cursor:pointer;
}

The icons aren't clickable so we shouldn't add a mouse pointer on hover. If we want a mouse pointer on hover, we should put the icons in a link <a>


60 changes: 60 additions & 0 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,71 @@
<link rel="stylesheet" href="css/style.css">
<link rel="shortcut icon" type="image/x-icon" href="favicon.ico">
</head>

<body>

<!-- Add your HTML markup here -->
<!-- Remember: Use semantic HTML tags like <header>, <main>, <nav>, <footer>, <section> etc -->
<!-- All the images you need are in the 'img' folder -->

<!-- header starts here-->
<header id="header">
<nav id="nav-bar">
Comment on lines +20 to +21
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<header id="header">
<nav id="nav-bar">
<header class="header">
<nav class="nav-bar">

Normally, software teams use classes for styling and not IDs. The same applies for the rest of this file

<img src= "img\karma-logo.svg" alt="Karma">
<ul class="nav">
<li> <a class="nav-link" href="#MeetKarma"> Meet Karma </a></li>
<li> <a class="nav-link" href="#Howitworks"> How it works </a> </li>
<li> <a class="nav-link" href="#Store"> Store </a> </li>
<li> <a class="nav-link" href="#Blog"> Blog </a> </li>
<li> <a class="nav-link" href="#Help"> Help </a></li>
<li> <a class="nav-link" href="#Login"> Login</a></li>
</ul>
</nav>
</header> <!--header ends here-->

<article>
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<article>
<main>

This seems like it should be a main, since we normally have header main footer as the top level elements on a page

<!--Introduction starts here-->
<section id="intro">
<div id="intro-content">
<h1> Introducing Karma </h1>
<h2> Bring WiFi with you, everywhere you go.</h2>
<button type="button" id="learnbutton"> Learn More </button>
</div>
</section>
<!-- Introduction ends here-->

<!--content starts here-->
<section id="content">
<h1> Everyone needs a little Karma.</h1>
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<h1> Everyone needs a little Karma.</h1>
<h2> Everyone needs a little Karma.</h2>

We should only have one h1 on the page, so we should update this to h2


<div class= "services">
<img class="service-img" src="img\icon-devices.svg" alt="Internet services"/>
<p > <a href = "" target="_blank" class="servicename" > Internet for all devices </a></p>
</div>

<div class= "services">
<img class="service-img" src="img\icon-coffee.svg" alt="Boost productivity"/>
<p class="services"> <a href = "" target="_blank" class="servicename" > Boost your productivity </a> </p>
</div>

<div class= "services">
<img class="service-img" src="img\icon-refill.svg" alt="Pay as You Go"/>
<p class="services"> <a href = "" target="_blank" class="servicename" > Pay as You Go </a> </p>
</div>
</section>
<!-- content ends here-->
</article>


<footer>
<p> Join us on </p>
Copy link
Copy Markdown
Contributor Author

@bonboh bonboh Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p> Join us on </p>
<h2> Join us on </h2>

This seems like it could be a heading, so we could make it one

<ul>
<li id="twitter"> <img src="img\twitter-icon.svg" alt="twitter"> </li>
<li id="facebook"> <img src="img\facebook-icon.svg" alt="facebook"> </li>
<li id="instagram"> <img src="img\instagram-icon.svg" alt="instagram"> </li>
</ul>
<p> &copy; Karma Mobility, Inc. </p>
</footer>

</body>
</html>