-
Notifications
You must be signed in to change notification settings - Fork 1
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
Redo game show page #124
base: integration
Are you sure you want to change the base?
Redo game show page #124
Conversation
@chrisdimoff @blb451 @jamesonji can you review this ticket? |
@kamork Hi Kitty. My pull request went in before yours. I had to change a few things to get multiple pictures to upload which will break your current carousel. To fix your carousel you should copy the code for the carousel from the integration branch and insert it into your newly designed show page. The crux of the issue is that we can no longer call game.picture . |
@kamork to add on to what @chrisdimoff said, you should prioritize the the carousel code from integration when resolving the merge conflict. However, keep your code for the rest of the page. Then, test if it all works. |
Added the new image carousel code, also made a few tweaks to divs and buttons. |
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.
Your PR looks good. However, the formatting of the HTML needs some sculpting. Chisel those corners and sand the edges! 🎨 🖌
@@ -12,7 +12,7 @@ | |||
<%= Tag.find(tagging.tag_id).name %> | | |||
<% end %> | |||
</p> | |||
<div class="container-fluid" style="background-color: steelblue; padding-top: 5px"> | |||
<div class="container-fluid" style="background-color: steelblue"> | |||
<%# if can?(:manage, @game) %> |
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.
Can you delete this line?
@@ -31,7 +31,6 @@ | |||
<% end %> | |||
</div> | |||
<% end %> | |||
|
|||
<%# if (can? :manage, @game ) %> |
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.
Delete this one too. It's not used 😎
@@ -1,97 +1,105 @@ | |||
<div class ="game-show"> |
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.
Can you remove the space between class =
? Bridge the divide! ✊
<div class="container-fluid"> | ||
<div class="row"> | ||
<div class="col-md-6 col-sm-6"> | ||
<h2 style="font-weight:bold; margin-bottom: 20px; color: #EF7030;"><%= @game.title %></h2> |
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.
Avoid inlining styles. Can put this in a class? You could use a selector such as .game-show h2
if it makes sense for the page seeing how this h2
is inside div.game-show
.
<% end %> | ||
</p> | ||
<div class="container-fluid" style="background-color: steelblue"> | ||
<%# if can?(:manage, @game) %> |
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.
Can you delete this line?
</div> | ||
</div> | ||
<% end %> | ||
</div> |
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.
The indent level on this closing div isn't quite right.
<% end %> | ||
</div> | ||
<!-- end of game management --> | ||
<div class="col-md-6 col-sm-6"> |
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.
Is this div used at all? Please fix the indenting. 🤓
<ol class="carousel-indicators"> | ||
|
||
<% @game.pictures.each_with_index do |pic, i| %> | ||
<li data-target="#home-slideshow" data-slide-to= <%= "#{i}" %> |
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.
Remove the space after data-slide-to=
<% @game.pictures.each_with_index do |picture, index| %> | ||
<% url = picture.carousel.url %> | ||
<% if index == 0 %> | ||
<div class="item active"> |
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.
Indenting should be two spaces
</div> | ||
<% end %> | ||
|
||
<!-- end of game stats --> | ||
<% if user_is_gamer? %> | ||
<div> |
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.
Too much indenting here.
@whilestevego please take a look