-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
[Feature] Copy to Clipboard for Answers #1461
[Feature] Copy to Clipboard for Answers #1461
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1461 +/- ##
=======================================
Coverage 95.41% 95.41%
=======================================
Files 174 174
Lines 2702 2702
=======================================
Hits 2578 2578
Misses 124 124 ☔ View full report in Codecov by Sentry. |
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.
Thanks for the contribution! This is already a good base, I left some suggestions that you should please take a look at!
app/helpers/social_helper.rb
Outdated
|
||
def answer_copy_content(answer) | ||
# copy content template: "question - answer [link]" | ||
"#{answer.question.content} - #{answer.content} [#{answer_share_url(answer)}]" | ||
end |
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.
You can just use the SocialHelper::TwitterMethods#prepare_tweet
method, which returns exactly the same format.
The [] around the link in the issue were also just for demonstration, they are not required in the actual output ;)
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.
I would suggest making a controller specifically for the clipboard feature, so we can reuse it in other places that don't necessarily have to do with sharing.
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.
I actually meant to request changes instead, sorry! (comments from above still apply)
app/helpers/social_helper.rb
Outdated
def answer_copy_content(answer) | ||
# copy content template: "question - answer [link]" | ||
"#{answer.question.content} - #{answer.content} [#{answer_share_url(answer)}]" | ||
end |
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.
RDoc comments go above the method instead of inside it.
def answer_copy_content(answer) | |
# copy content template: "question - answer [link]" | |
"#{answer.question.content} - #{answer.content} [#{answer_share_url(answer)}]" | |
end | |
# copy content template: "question - answer link" | |
def answer_copy_content(answer) | |
"#{answer.question.content} - #{answer.content} #{answer_share_url(answer)}" | |
end |
This can also be shortened using end-less method definition like so:
def answer_copy_content(answer) | |
# copy content template: "question - answer [link]" | |
"#{answer.question.content} - #{answer.content} [#{answer_share_url(answer)}]" | |
end | |
# copy content template: "question - answer link" | |
def answer_copy_content(answer) = "#{answer.question.content} - #{answer.content} #{answer_share_url(answer)}" |
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.
- Thanks for your suggestion. I'd keep this in mind for my future PRs.
- However, as per this comment, I'll be removing this method. Thanks. :)
Hey @pixeldesu, @raccube - Thanks for your suggestions. I've addressed them. It's ready for review. Thanks :) |
Hey @pixeldesu - A soft reminder for this. Please let me know if I'm missing anything here. Thanks. |
@chahmedejaz I already had a look at the code, but I want to test it locally before the next review. I am quite busy right now so I'll try to review it by the weekend! |
Oh no problem then. 😄 |
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.
So I reviewed this, and the code looks and works fine! Good job 🎉
After testing, I have two more things that should be addressed:
- The copy action doesn't really have feedback, it feels like I don't really know pressing the button copied the content to my clipboard, we should probably show a notification toast (using
showNotification
) here!* - The initial feature request was meant to be for the sharing dialog that comes up after answering a question in the inbox. So we should add the button there as well, so people can copy the answer to their clipboard right after answering.
*Some notes for adding translations in the JS:
- All strings that should be accessible need to be present in the
frontend.*.yml
file. - For it to appear in the frontend, you then need to run
rails i18n export
beforeyarn run build
, so the new translations are actually packaged.
Sorry again for taking so long with the review, we really appreciate your recent contributions! :)
Thank you so much for the feedback and the guide, @pixeldesu 😄 What's the notification message would you prefer here? I was thinking of: |
That reads fine in my opinion. |
Hey @pixeldesu - I've addressed your suggestions. Please review and let me know if I'm missing anything. Thanks. |
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.
👍 Awesome, everything's in order now! Good work!
Resolves: #1448
Screenshot:
![image](https://private-user-images.githubusercontent.com/59338032/282268708-20047de0-f3db-4cbc-9f30-0cd446ddeb8a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg5MjUwNzYsIm5iZiI6MTczODkyNDc3NiwicGF0aCI6Ii81OTMzODAzMi8yODIyNjg3MDgtMjAwNDdkZTAtZjNkYi00Y2JjLTlmMzAtMGNkNDQ2ZGRlYjhhLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMDclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjA3VDEwMzkzNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWYwOTZiMDQ5ZjNiYmUzM2M2NDY3ZTE0ZTVhNzRlNjE4OWRmODVkNGYyOGUyZjdlYTgyNGU4NjM5N2M5MDVhOTkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.Gs5Zvp39c9MdCs5ZufQZ_QHU9iREcxuRYtD5D3KXhfA)