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
update likes with emoji_type: nil to emoji_type: ThumbsUp #2999
Conversation
Generated by 🚫 Danger |
want to try pushing to unstable? 🎉 |
Sure! |
Pushing on unstable! |
79e40c9
to
8085957
Compare
@jywarren the build went well on jenkins but it shows some wierd error--something related to memory |
|
@jywarren any ideas on this? The build went fine but shows error on travis. Locally this worked perfectly. |
aha - i've seen this one -- try restarting it!
…On Fri, Jul 6, 2018 at 4:37 PM Vidit ***@***.***> wrote:
@jywarren <https://github.com/jywarren> any ideas on this? The build went
fine but shows error on travis. Locally this worked perfectly.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2999 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJySdKSqVj9WSyc3A5zqdZxN8sHDlks5uD8p4gaJpZM4VFsvX>
.
|
tried already! Gives the same error!
On Sat, Jul 7, 2018 at 2:17 AM Jeffrey Warren <notifications@github.com>
wrote:
… aha - i've seen this one -- try restarting it!
On Fri, Jul 6, 2018 at 4:37 PM Vidit ***@***.***> wrote:
> @jywarren <https://github.com/jywarren> any ideas on this? The build
went
> fine but shows error on travis. Locally this worked perfectly.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#2999 (comment)>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AABfJySdKSqVj9WSyc3A5zqdZxN8sHDlks5uD8p4gaJpZM4VFsvX
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2999 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AVkX0baa8F6kTfyPvADP5wxFistAtk8Iks5uD8zygaJpZM4VFsvX>
.
|
whoa... let me try one more time. Hmm. |
Wohoo, passed this time!! That's really cool! |
Hm, I guess I'm still wondering if standardizing to the standard emoji short code value would be a good idea: https://www.emojibase.com/emoji/1f44d/thumbsupsign I don't know though. If you think this is good, i'll merge! Thanks! |
Sure, no problems. I'll make the changes. |
Hi @jywarren, I tried replacing ThumbsUp with +1 which is when I realised that the logic becomes a lot more clumsy due to this. First of all the + in "+1" is ignored in query params, which makes it just " 1". It is also ignored when I try to use instance variables (e.g. '@likes') inside views. And I just realised github uses "ThumbsUp" too, maybe the reason being the same. This is doable but we might need to handle a lot of cases for this. |
ah, that makes a lot of sense! I'd say let's just do ThumbsUp - or maybe
:thumbs_up: since we see a lot in that syntax (:raised_hands: and such) --
thanks! And if there's a convenient place to note this in a comment, let's
do.
…On Sun, Jul 8, 2018 at 11:37 AM Vidit ***@***.***> wrote:
Hi @jywarren <https://github.com/jywarren>, I tried replacing ThumbsUp
with +1 which is when I realised that the logic becomes a lot more clumsy
due to this. First of all the + in "+1" is ignored in query params, which
makes it just " 1". It is also ignored when I try to use instance variables
(e.g. ***@***.*** <https://github.com/likes>') inside views. And I just
realised github uses "ThumbsUp" too, maybe the reason being the same. This
is doable but we might need to handle a lot of cases for this.
Your thoughts on this??
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2999 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ3VqJ3WHdxwQ7Heb7n7MyVKjysUZks5uEicLgaJpZM4VFsvX>
.
|
Cool, so shall we go forward with this?? Or any other changes I need to make? We need to make it ThumbsUp instead of +1 for comment autocomplete. Please correct me if I'm wrong. |
I think let's change it to |
Hmm, I have kept it |
ok awesome, good to go now? Thanks @ViditChitkara !!! |
Yes!! |
Awesome!!! 🎉 🎉 🎉 |
Thanks a lot! |
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test
fixes #0000
-style reference to original issue #@publiclab/reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.
Thanks!