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

Add view button for every displayed Bet in /bets #37

Merged
1 commit merged into from
Apr 5, 2017
Merged

Add view button for every displayed Bet in /bets #37

1 commit merged into from
Apr 5, 2017

Conversation

ghost
Copy link

@ghost ghost commented Mar 13, 2017 •
edited by ghost
Loading

Pivotal: https://www.pivotaltracker.com/story/show/141340981
Dependent on #60, #61, #62
The button takes the user to the appropriate Bet show page.

jacquesporveau
jacquesporveau previously requested changes Mar 14, 2017
View reviewed changes
app/views/bets/_display_bets.html.erb Outdated
<%=
link_to bet, class: 'btn btn-secondary', role: 'button' do
content_tag :i, class: 'fa fa-eye' do
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the do end is needed here for your content tag as you aren't sending in anything for the block.

http://apidock.com/rails/ActionView/Helpers/TagHelper/content_tag

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.
To use content_tag without a block you have to pass it a third argument though:
content_tag :i, nil, class: 'stuff'

In this case you shouldnt use content_tag. None of your values are dynamic. It would be more clear to write
<i class="stuff"></i>

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it bombs without the do...end. Didn't know you could one line it with passing nil though, thanks!
Now, if you excuse me, I have 5 thousand other "content_tag"s to fix.

@ghost ghost force-pushed the users_can_access_any_bet_from_view_bets_page_raw branch from 7c821cd to 2a2b8de Compare March 14, 2017 16:16
@ghost ghost force-pushed the view_bets_and_pagination_commits branch 2 times, most recently from 12d9683 to 81f420c Compare March 16, 2017 21:35
@qr8r
Copy link
Contributor

qr8r commented Mar 16, 2017

Jenkins test this please

@cbrunsdon
Copy link
Member

jenkins test this please

@ghost ghost force-pushed the view_bets_and_pagination_commits branch 10 times, most recently from 405cde9 to 5db032c Compare March 27, 2017 23:46
@ghost ghost force-pushed the users_can_access_any_bet_from_view_bets_page_raw branch from 2a2b8de to bdae7a2 Compare March 28, 2017 20:24
@ghost ghost changed the base branch from view_bets_and_pagination_commits to master March 28, 2017 20:24
@ghost ghost changed the base branch from master to display_all_bets March 28, 2017 20:25
@ghost ghost changed the base branch from display_all_bets to master March 28, 2017 20:26
@ghost ghost force-pushed the users_can_access_any_bet_from_view_bets_page_raw branch from bdae7a2 to 26a724b Compare March 28, 2017 20:28
@ghost ghost changed the base branch from master to display_all_bets March 28, 2017 20:28
@ghost ghost changed the base branch from display_all_bets to master March 28, 2017 20:29
@ghost ghost force-pushed the users_can_access_any_bet_from_view_bets_page_raw branch from 26a724b to 92e6c13 Compare March 28, 2017 20:53
@ghost ghost changed the base branch from master to display_all_bets March 28, 2017 20:53
@ghost ghost force-pushed the users_can_access_any_bet_from_view_bets_page_raw branch from 92e6c13 to baa0a80 Compare March 28, 2017 20:56
@ghost ghost force-pushed the display_all_bets branch from 733fd12 to 19e50bd Compare March 29, 2017 17:29
@jacquesporveau
Copy link
Member

This branch has merge conflicts. Please resolve them.

@ghost ghost force-pushed the users_can_access_any_bet_from_view_bets_page_raw branch from baa0a80 to 9253769 Compare March 29, 2017 22:31
@ghost ghost changed the base branch from display_all_bets to master March 29, 2017 22:31
qr8r
qr8r reviewed Mar 31, 2017
View reviewed changes
app/views/bets/_bet.html.erb Outdated
<td class="col-md-3"><%= bet.bet %></td>
<td class="col-md-1">
<%= link_to bet, class: 'btn btn-secondary', role: 'button' do %>
<div style="font-size: 0.8em;" class="text-center">
Copy link
Contributor

Choose a reason for hiding this comment

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

👎 Inline styles

@ghost ghost force-pushed the users_can_access_any_bet_from_view_bets_page_raw branch from 9253769 to 161408d Compare March 31, 2017 17:04
@LeonTenko
Add view button for every displayed Bet
df3143b 
The button takes the user to the appropriate Bet show page.
@ghost ghost force-pushed the users_can_access_any_bet_from_view_bets_page_raw branch from 161408d to df3143b Compare April 5, 2017 21:09
@ghost ghost merged commit d876757 into master Apr 5, 2017
@ghost ghost deleted the users_can_access_any_bet_from_view_bets_page_raw branch April 12, 2017 17:21
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qr8r @cbrunsdon @jacquesporveau @LeonTenko