Skip to content
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

Jump to bottom

Get ready #29

Merged
merged 6 commits into from
Mar 29, 2017
Merged

Get ready #29

merged 6 commits into from
Mar 29, 2017

Conversation

V-Ho
Copy link
Collaborator

@V-Ho V-Ho commented Mar 28, 2017 •
edited by Winkeltje84
Loading

#21
get ready screenshot

@V-Ho V-Ho requested a review from Winkeltje84 March 28, 2017 09:45
@Winkeltje84
Copy link
Owner

if you look at the assignment it looks like:
screen shot 2017-03-28 at 11 58 19

but yours looks like this:
screen shot 2017-03-28 at 11 58 37

The three need to be centralized a little bit more.
And I think the p needs to be made smaller.
But this is all styling for later

@Winkeltje84
Copy link
Owner

And don't forget "Get ready for the day" & "Know your tools" ;-)

arnoFleming
arnoFleming reviewed Mar 28, 2017
View reviewed changes
Copy link
Contributor

@arnoFleming arnoFleming left a comment

Choose a reason for hiding this comment

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

Styling is important - do it now, or create a ticket for it to do it before friday!

So, if you follow either of those to paths, it LGTM and you can 🚢 it

@V-Ho
Copy link
Collaborator Author

V-Ho commented Mar 28, 2017

Final styling changes:
screenshot from 2017-03-28 14-25-15

Winkeltje84
Winkeltje84 approved these changes Mar 28, 2017
View reviewed changes
Copy link
Owner

@Winkeltje84 Winkeltje84 left a comment

Choose a reason for hiding this comment

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

looks great now!

app/views/getready.html.erb Outdated
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u" crossorigin="anonymous">
</head>

<body>
Copy link
Owner

Choose a reason for hiding this comment

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

think you can leave the body out, that will be in the main html file

@Winkeltje84
Copy link
Owner

@miriamtocino , can you LGTM t(w)oo'?

leefreemanxyz
leefreemanxyz reviewed Mar 28, 2017
View reviewed changes
app/views/getready.html.erb Outdated
@@ -0,0 +1,43 @@
<!DOCTYPE html>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should be renamed to _get_ready.html.erb (because Ruby programmers love snake_case)

@leefreemanxyz leefreemanxyz mentioned this pull request Mar 28, 2017
Closed
@Winkeltje84 Winkeltje84 added LGTM1 and removed LGTM1 labels Mar 28, 2017
@V-Ho V-Ho merged commit e935507 into master Mar 29, 2017
@leefreemanxyz leefreemanxyz deleted the get-ready branch March 29, 2017 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leefreemanxyz leefreemanxyz leefreemanxyz left review comments

@arnoFleming arnoFleming arnoFleming left review comments

@Winkeltje84 Winkeltje84 Winkeltje84 approved these changes

@miriamtocino miriamtocino Awaiting requested review from miriamtocino

Assignees
No one assigned
Labels
LGTM1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@V-Ho @Winkeltje84 @arnoFleming @leefreemanxyz