Ajax notifications #2829
Ajax notifications #2829
Conversation
ghost
assigned 
namangupta01
force-pushed
the
ajax_notifications
branch
from
51be01a
to
918add2
Compare
Generated by 🚫 Danger |
app/assets/javascripts/like.js
Outdated
| $('#like-button-' + node_id).on('click', clicknotliked); | ||
| $('#like-button-' + node_id).off('click', clickliked); | ||
| new Noty({ | ||
| theme: 'mint', |
There was a problem hiding this comment.
Could we reduce repetition by centralizing our default settings into javascripts/noty.js in a variable, and then this could become a single line?
| @@ -11,6 +11,8 @@ | |||
| <% end %> | |||
| <meta name="author" content="Public Lab contributors" /> | |||
| <link href="https://<%= request.host %>/feed.rss" rel="alternate" type="application/rss+xml" title="Public Lab research" /> | |||
| <link href="/lib/noty/lib/noty.css" rel="stylesheet"> | |||
| <script src="/lib/noty/lib/noty.js" type="text/javascript"></script> | |||
There was a problem hiding this comment.
And if this is on every page, we could get it compiled in, at /app/assets/javascripts/application.js and /app/assets/stylesheets/application.css
bower.json
Outdated
| @@ -26,7 +26,8 @@ | |||
| "short-code-forms": "jywarren/short-code-forms#~0.0.1", | |||
| "leaflet-blurred-location": "publiclab/leaflet-blurred-location#master", | |||
| "chart.js": "v2.7.0", | |||
| "typeahead.js-browserify": "Javier-Rotelli/typeahead.js-browserify#~1.0.7" | |||
| "typeahead.js-browserify": "Javier-Rotelli/typeahead.js-browserify#~1.0.7", | |||
| "noty": "3.1.4" | |||
There was a problem hiding this comment.
Can you also add this to the Yarn PR so we don't break it in the future -- in #2659 ? Thanks!
|
Here's the gif |
|
@jywarren Please check if i correctly added the |
app/views/comment/delete.js.erb
Outdated
| } |
There was a problem hiding this comment.
Sorry just checking - are we missing a closing ) here?
| @@ -0,0 +1,11 @@ | |||
| function notyNotification(theme, timeout, type, layout, text){ | |||
|
@jywarren do we need to adjust code climate ? |
|
i approved them. good to go? 😄 |
|
Yeah! 👍 |
|
Is it good to go? Does this new delete design is fine acc. to you? |
|
Ack sorry, looks like it needs a rebase again? Otherwise great! |
namangupta01
force-pushed
the
ajax_notifications
branch
from
ae3c047
to
5d13f77
Compare
| $("#c<%= comment.cid %>").remove() | ||
| $('#comment-count')[0].innerHTML = parseInt($('#comment-count')[0].innerHTML)-1 | ||
| }) | ||
| $("#c<%= comment.cid %>delete-btn-2").bind('ajax:success', function(e,response){ |
There was a problem hiding this comment.
@jywarren I think this was redundant. What do you think?
There was a problem hiding this comment.
@ViditChitkara As you are working a lot on commenting i think you may have idea about this...is this redundant here?
|
I wonder if @ViditChitkara could take a look through this code, having recently worked a lot on commenting? |
|
yes but after rebasing this needs some work. Will update this pr in some time. |
namangupta01
force-pushed
the
ajax_notifications
branch
from
5d13f77
to
231b07e
Compare
|
@jywarren It is now good to merge. You can also test this on unstable. |
|
Awesome, thanks!!! |
* Added notyjs for ajax call notifications * Added notyjs for ajax call notifications and added jquery-confirm for confimation modal * Added notyjs for ajax call notifications and added jquery-confirm for confimation modal * Minor Changes * Minor changes * Changed notification text * Minor changes * Minor change
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
Closes #2277
rake testfixes #0000-style reference to original issue #@publiclab/reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto 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!