-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Added topbanner #27
Added topbanner #27
Conversation
app/views/topbanner.html.erb
Outdated
<section> | ||
<div class="container"> | ||
<div class="jumbotron" align="middle"> | ||
<img src="http://res.cloudinary.com/dqaserzum/image/upload/v1490689989/Screen_Shot_2017-03-28_at_10.29.07_pv9ybs.png" height="125" alt="'Topp banner Taste of Code" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"'Topp banner Taste of Code" there is a small typo here, maybe change it to "Top banner Taste of Code"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to render images that you added yourself, please add them to your app/assets/images
and use image_tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added image to image folder now and reffered to it in html
app/views/topbanner.html.erb
Outdated
<div class="container"> | ||
<div class="jumbotron" align="middle"> | ||
<img src="http://res.cloudinary.com/dqaserzum/image/upload/v1490689989/Screen_Shot_2017-03-28_at_10.29.07_pv9ybs.png" height="125" alt="'Topp banner Taste of Code" /> | ||
<p class="lead" style="padding:10px">Beginner Coding Workshop</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the wireframe it also says 'Taste of Code' above 'Beginner Coding Workshops' please add that too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added it, but title is already in Logo, so we would only want to show it if it is not in the logo I think
Also I almost forgot: is this a partial? Because in that case I think the filename needs an _ at the start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good Bas! Nice job with the banner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress! 💪
A small comment.
When that's done, you can merge it!
app/views/shared/_topbanner.html.erb
Outdated
<head> | ||
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u" crossorigin="anonymous"> | ||
</head> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a partial you don't need the <head>
tag. That's only in your layout.
Please remove it from here 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, did that to make the page visible by just looking at that single html file. Will delete it!
looks like this now
![screen shot 2017-03-28 at 14 03 13](https://cloud.githubusercontent.com/assets/15578000/24403934/61ed4848-13bf-11e7-9930-0132dea8081d.png)