Facebook share button working by grvsachdeva · Pull Request #152 · rubyforgood/playtime

grvsachdeva · 2017-10-21T19:00:17Z

Resolves issue #112

I have used social-share-button gem only to get the facebook share functionality to work by changing some syntax. I have added URL and desc attributes.
Example of the run at localhost

@leesharma please have a look and suggest if any improvements required. Thank You.

leesharma

@Gauravano Thanks for the PR! 🎉

This looks pretty good (especially that html fix–thanks for that!) I've got two requests before merging:

It looks like you're using RubyMine/IntelliJ and checked in the config files by accident (the .idea directory). Could you please remove those from the commit? Feel free to add .idea to .gitignore if it helps.
It looks like you've hard-coded the staging URL. We'll want this functionality to work locally, on staging, and (especially) in production, and all of those use different URLs. Is there a way to make that work without specifying a URL? If not, could we use environment variables?

Thanks again for the PR, and let me know if you have any questions about the review comments!

leesharma · 2017-10-21T20:20:19Z

.idea/.generators

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>


Could you remove the RubyMine files (.idea/*) from your PR? If it'd be helpful to add .idea to .gitignore, feel free to do that!

leesharma · 2017-10-21T20:20:46Z

app/views/wishlists/new.html.erb

@@ -1,6 +1,6 @@
 <div class="container">
  <div class="header">
-    <h2>Create a New Wishlist</h1>


Good catch! 👍

leesharma · 2017-10-21T20:22:08Z

app/views/wishlists/show.html.erb

  <h2>
    <%= @wishlist.name %>
-    <%= social_share_button_tag("Are you a hero of play? #{@wishlist.name} needs your help!") %>
+    <%= social_share_button_tag("Are you a hero of play? #{@wishlist.name} needs your help!",:url => "https://project-playtime-staging.herokuapp.com/wishlists/#{@wishlist.id}",desc: "Are you a hero of play? #{@wishlist.name} needs your help!") %>


This URL changes. Is there a way to do this without a url attribute? If not, can we get this using environment variables?

leesharma

Thanks for the update! This is closer, but there are still some issues (details in comments):

It looks like this changes the ruby version to 2.3.3; we don't want to do that.
This still hard-codes URLs. Maybe the #wishlist_url helper would work here?
I also noted some style issues, but they should be easy fixes.

Again, thanks, and let me know if you have any questions!

leesharma · 2017-10-22T22:01:20Z

app/views/wishlists/show.html.erb

+        <% url_share = "https://project-playtime-staging.herokuapp.com/wishlists/#{@wishlist.id}" %>
+    <% end %>
+
+    <%= social_share_button_tag("Are you a hero of play? #{@wishlist.name} needs your help!",:url => url_share,desc: "Are you a hero of play? #{@wishlist.name} needs your help!") %>


Some style nitpicks:

You're using two different hash formats here (:url => ... and desc: ...); let's stick to the second one (key: value).

For readability, either add spaces after the commas or break this up into multiple lines.

i will take care of it

leesharma · 2017-10-22T22:01:56Z

Gemfile.lock


 RUBY VERSION
-   ruby 2.4.1p111
+   ruby 2.3.3p222


We don't want to change our ruby version.

leesharma · 2017-10-22T22:04:32Z

app/views/wishlists/show.html.erb

+        <% url_share = "https://project-playtime-staging.herokuapp.com/wishlists/#{@wishlist.id}" %>
+    <% else %>
+        <% url_share = "https://project-playtime-staging.herokuapp.com/wishlists/#{@wishlist.id}" %>
+    <% end %>


This still hard-codes URLs; we want this to work anywhere it's hosted.

Actually, looking at it, the url helpers would probably work: wishlist_url(@wishlist). Would that be an easier solution?

Making helper is beneficial if wishlist_url has to be used at many placed otherwise I can optimise this URL generally too

leesharma

This looks really good! One last change request: rails actually has built-in _url helpers; could you remove the custom helper method? (more details in comments)

After that's done, I think this is ready to merge. 😄

leesharma · 2017-11-06T02:46:33Z

app/helpers/application_helper.rb


+  def wishlist_url(wishlist)
+     request.base_url+"/wishlists/#{wishlist.id}"
+  end


This is actually built into Rails, so you can remove this method. As an added bonus, the pre-included method will match your app configuration without extra work (ex. with subdomains, ports, friendly urls/slugs, etc.)

Here's a section of the official Rails Guides talking about _path and _url helpers.

leesharma · 2017-11-06T02:47:09Z

app/views/wishlists/show.html.erb

-    <%= social_share_button_tag("Are you a hero of play? #{@wishlist.name} needs your help!",:url => url_share,desc: "Are you a hero of play? #{@wishlist.name} needs your help!") %>
+    <%= social_share_button_tag("Are you a hero of play? #{@wishlist.name} needs your help!",
+                                url: wishlist_url(@wishlist),
+                                desc: "Are you a hero of play? #{@wishlist.name} needs your help!") %>


Looks good! 👍 I like how you broke this up into multiple lines. Much easier to read!

leesharma · 2017-11-06T03:59:46Z

Gemfile.lock


 RUBY VERSION
-   ruby 2.3.3p222
+   ruby 2.4.1p111


Facebook share button working

7385abf

leesharma requested changes Oct 21, 2017

View reviewed changes

grvsachdeva added 2 commits October 22, 2017 13:39

URL updation according to Environment

9f76523

URL and gitignore update

9dbe111

leesharma requested changes Oct 22, 2017

View reviewed changes

general link using helper

2f36c53

micahbales temporarily deployed to project-playtime-stagin-pr-152 November 6, 2017 02:48 Inactive

leesharma reviewed Nov 6, 2017

View reviewed changes

micahbales temporarily deployed to project-playtime-stagin-pr-152 November 21, 2017 03:33 Inactive

Conversation

grvsachdeva commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leesharma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leesharma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grvsachdeva Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leesharma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grvsachdeva commented Oct 21, 2017 •

edited

Loading

grvsachdeva Oct 24, 2017 •

edited

Loading