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.

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

Change comment count on comment creation via AJAX #441

Closed
5 tasks
grvsachdeva opened this issue Mar 23, 2019 · 7 comments
Closed
5 tasks

Change comment count on comment creation via AJAX #441

grvsachdeva opened this issue Mar 23, 2019 · 7 comments

Comments

@grvsachdeva
Copy link
Member

Hi, this is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

We know that the process of creating a pull request is the biggest barrier for new contributors. This issue is for you 💝

If you have contributed before, consider leaving this one for someone new, and looking through our general help wanted issues. Thanks!

🤔 What you will need to know.

Nothing. This issue is meant to welcome you to Open Source :) We are happy to walk you through the process.

📋 Step by Step

  • 🙋 Claim this issue: Comment below. If someone else has claimed it, ask if they've opened a pull request already and if they're stuck -- maybe you can help them solve a problem or move it along!

Currently, the comment addition at https://mapknitter.org/maps/test-map-3 takes place using AJAX so no refresh happens. So, it would be better if we also increment the comment count too.

See this page for some help in taking your first steps!

Below is a "diff" showing in red (and a -) which lines to remove, and in green (and a +) which lines to add:

+    $("#comments-number").text(function(i, str) { return (parseInt(str) + 1); });

With this code addition, with every new comment, the comment count will be incremented at the same time.

  • 💾 Commit your changes

  • 🔀 Start a Pull Request. There are two ways how you can start a pull request:

  1. If you are familiar with the terminal or would like to learn it, here is a great tutorial on how to send a pull request using the terminal.

  2. You can also edit files directly in your browser and open a pull request from there.

  • 🏁 Done Ask in comments for a review :)

Please keep us updated

💬⏰ - We encourage contributors to be respectful to the community and provide an update within a week of claiming a first-timers-only issue. We're happy to keep it assigned to you as long as you need if you update us with a request for more time or help, but if we don't see any activity a week after you claim it we may reassign it to give someone else a chance. Thank you in advance!

If this happens to you, don't sweat it! Grab another open issue.

Is someone else already working on this?

🔗- We encourage contributors to link to the original issue in their pull request so all users can easily see if someone's already started on it.

👥- If someone seems stuck, offer them some help! Otherwise, take a look at some other issues you can help with. Thanks!

🤔❓ Questions?

Leave a comment below!

@grvsachdeva
Copy link
Member Author

This issue is created for @sonali9696. Thanks!

@sonali9696
Copy link
Member

sonali9696 commented Mar 23, 2019

Hey @gauravano,

Thanks a lot for creating an issue for me so quickly! I deeply appreciate it. I understood what changes to do and can go ahead and raise the PR but before that, I wanted to understand the code better and try to test it. However, the link which is given above: https://mapknitter.org/maps/test-map-3 is showing "Page doesn't exist" for me. I have raised the issue for the same here: #442

image

@sonali9696
Copy link
Member

If you find some time, it would be great if you could clarify my doubt:

While deleting a comment, the code used is:
$("#comments-number").text(function(i, str) { return str - 1; });

However the code you asked to add for incrementing comments-number is:
$("#comments-number").text(function(i, str) { return (parseInt(str) + 1); });

Is parseInt necessary here? If yes, why didn't it cause an issue in the code used in delete_comment function? I went to the scripts mentioned in "require" at the top of the code and the codes they in turn led to but I didn't find any references or usage of the text or str or comments-number.

sonali9696 added a commit to sonali9696/mapknitter that referenced this issue Mar 23, 2019
This closes issue publiclab#441 "Change comment count on comment creation via AJAX publiclab#441" by incrementing comments-number each time a new comment is added. This would ensure that the counter indicating the number of comments is increased without needing to refresh the page.
@grvsachdeva
Copy link
Member Author

grvsachdeva commented Mar 23, 2019

Thanks a lot for creating an issue for me so quickly! I deeply appreciate it. I understood what changes to do and can go ahead and raise the PR but before that, I wanted to understand the code better and try to test it. However, the link which is given above: https://mapknitter.org/maps/test-map-3 is showing "Page doesn't exist" for me. I have raised the issue for the same here: #442

hey @sonali9696, actually the link- https://mapknitter.org/maps/test-map-3 I provided was an example for representing the route(sorry I should be more specific), you wouldn't find that on production because there is no map with title test-map-3 in production.

But, you can reproduce this issue by commenting at https://mapknitter.org/maps/testing--8(It's a test map, I guess, it would be deleted shortly by moderators but feel free to comment for testing on this one).
You can also reproduce the issue by running the project locally - https://github.com/publiclab/mapknitter/#installation too.

Thanks!

@grvsachdeva
Copy link
Member Author

Is parseInt necessary here?

yes!

If yes, why didn't it cause an issue in the code used in delete_comment function?

Because, if I initialize a variable a = "3" and perform operation as a = a-1, it will return 2 as a result. But, if I perform + operation on variable a = "8" i.e., a = a + 1. I will get "81" as a result as JS will treat them as a string and will perform concatenation.

Screenshot from 2019-03-23 23-57-30

I went to the scripts mentioned in "require" at the top of the code and the codes they in turn led to but I didn't find any references or usage of the text or str or comments-number.

We are fetching the number of comments from DOM using jQuery as $("#comments-number").text() gives us the number of comments in string format.

grvsachdeva pushed a commit that referenced this issue Mar 23, 2019
This closes issue #441 "Change comment count on comment creation via AJAX #441" by incrementing comments-number each time a new comment is added. This would ensure that the counter indicating the number of comments is increased without needing to refresh the page.
@grvsachdeva
Copy link
Member Author

Solved in #443

@sonali9696
Copy link
Member

Thank you for taking out the time to clarify my doubts :)

icarito pushed a commit that referenced this issue Apr 7, 2019
This closes issue #441 "Change comment count on comment creation via AJAX #441" by incrementing comments-number each time a new comment is added. This would ensure that the counter indicating the number of comments is increased without needing to refresh the page.
chen-robert pushed a commit to chen-robert/mapknitter that referenced this issue Dec 5, 2019
…iclab#443)

This closes issue publiclab#441 "Change comment count on comment creation via AJAX publiclab#441" by incrementing comments-number each time a new comment is added. This would ensure that the counter indicating the number of comments is increased without needing to refresh the page.
jywarren added a commit that referenced this issue May 5, 2020
* Shortening docker image in ~30%

* Caching bundle, gathering env variables and using newer sintax

* Creating startup script and env file

* Improving travis CI configuration

* Loading assets in production env

* Allow uglifier to interpret ES6

* Fix start command

* Fix travis script

* Tweak travis script

* Add delay

* Revert assets changes

* Return to Mysql5.7

* Tweak travis script

* Fix make redeploy-container command

* Add db migrate and precompile step.

* Add bower install to Makefile

* Clean after docker run. Avoid one bower run.

* Changes to be able to build container in Google Cloud

* Remove spurious symlink

* Copy config examples when making build

* Export env variable name

* Tag cloud image

* Add timeout

* Push to cloud registry

* Fix jenkins build error with docker-compose tty

* Add app to container and .dockerignore all else

* Copy configuration files when deploying to GCE

* Allow copy config to container

* Time extended (for cloud build & push)

* Delete redundant index.html.erb file (#427)

* Setupcoveralls (#438)

* Add coveralls

* Fix gemfile

* Fix env variable

* Add coveralls token

* Update README.md

* Remove legacy image controller code #404 (#417)

Deleted the lines from the selection indicated in the issue.

* Change comment count on comment creation via AJAX #441 (#443)

This closes issue #441 "Change comment count on comment creation via AJAX #441" by incrementing comments-number each time a new comment is added. This would ensure that the counter indicating the number of comments is increased without needing to refresh the page.

* update syntax of active record query(license method) (#439)

Fixes #437

* Docker improve rebased (#450)

* Shortening docker image in ~30%

* Caching bundle, gathering env variables and using newer sintax

* Creating startup script and env file

* Improving travis CI configuration

* Loading assets in production env

* Allow uglifier to interpret ES6

* Don't dettach when building container in travis

* Fix start command

* Fix travis script

* Try to resolve travis tests invocation

* Tweak travis script

* Add delay

* Bundle install before db setup

* Shortening docker image in ~30%

* Caching bundle, gathering env variables and using newer sintax

* Creating startup script and env file

* Improving travis CI configuration

* Loading assets in production env

* Allow uglifier to interpret ES6

* Fix start command

* Fix travis script

* Tweak travis script

* Add delay

* Revert assets changes

* Return to Mysql5.7

* Tweak travis script

* Fix make redeploy-container command

* Add db migrate and precompile step.

* Add bower install to Makefile

* Clean after docker run. Avoid one bower run.

* updte pr template (#448)

* Bump recaptcha from 4.13.1 to 4.13.2 (#452)

Bumps [recaptcha](https://github.com/ambethia/recaptcha) from 4.13.1 to 4.13.2.
- [Release notes](https://github.com/ambethia/recaptcha/releases)
- [Changelog](https://github.com/ambethia/recaptcha/blob/master/CHANGELOG.md)
- [Commits](ambethia/recaptcha@v4.13.1...v4.13.2)

Signed-off-by: dependabot[bot] <[email protected]>

* Restructure rake test task runner (#380)

* add a mysql setup file

* Squash commits

* Update README.md (#456)

* Change run to exec (#457)

* Bump paperclip from 4.2.4 to 4.3.7 (#285)

Bumps [paperclip](https://github.com/thoughtbot/paperclip) from 4.2.4 to 4.3.7.
- [Release notes](https://github.com/thoughtbot/paperclip/releases)
- [Changelog](https://github.com/thoughtbot/paperclip/blob/v4.3.7/NEWS)
- [Commits](thoughtbot/paperclip@v4.2.4...v4.3.7)

Signed-off-by: dependabot[bot] <[email protected]>

* Bump test-unit from 3.3.0 to 3.3.1 (#458)

Bumps [test-unit](https://github.com/test-unit/test-unit) from 3.3.0 to 3.3.1.
- [Release notes](https://github.com/test-unit/test-unit/releases)
- [Commits](test-unit/test-unit@3.3.0...3.3.1)

Signed-off-by: dependabot[bot] <[email protected]>

* Bump coveralls from 0.7.1 to 0.8.22 (#453)

Bumps [coveralls](https://coveralls.io) from 0.7.1 to 0.8.22.

Signed-off-by: dependabot[bot] <[email protected]>

* gridview aligned (#464)

* Alert improvement and adding byebug gem (#383)

* byebug gem added and alerts in separate file

* adding byebug history to gitignore

* adding timestamp to redirect

* added z-index to render login dropdown above leaflet icon

* fixed image partial rendering when no images (#423)

* fixed image partial rendering when no images.

* toggle no images <p> om upload

* fixed image partial rendering when no images.

* Bump recaptcha from 4.13.2 to 4.14.0 (#471)

Bumps [recaptcha](https://github.com/ambethia/recaptcha) from 4.13.2 to 4.14.0.
- [Release notes](https://github.com/ambethia/recaptcha/releases)
- [Changelog](https://github.com/ambethia/recaptcha/blob/master/CHANGELOG.md)
- [Commits](ambethia/recaptcha@v4.13.2...v4.14.0)

Signed-off-by: dependabot[bot] <[email protected]>

* add a flash error when adding tags and not logged in (#473)

* Upgrade app to Bootstrap 4 (#480)

* Bootstrap 4 small button fixes (#488)

* Add tests for comments and maps (#467)

* Updated query style (#436) (#469)

* Dynamic ports (#462)

* Dynamic port in compose file

* Omit setting container name

* Add initial sql dump entry

* Avoid resetting database on build

* Shortening docker image in ~30%

* Caching bundle, gathering env variables and using newer sintax

* Creating startup script and env file

* Improving travis CI configuration

* Loading assets in production env

* Tweak travis script

* Roll back to using Debian 9 with custom built GDAL (#489)

* Switch back to Debian 9 Stretch

* Simplify docker image

* Bump Ruby to 2.4.6

* Re-add dependency

* Add dependency (zip)

* Try pip install gdal

* Install libgdal-dev

* Revert attept to use pip

* Bump ruby

* Avoid naming containers in compose file

* Avoid overwriting database on redeploy-container

* Allow to load mysql dump

* Include own GDAL packages

* Disable ipv6 to prevent error

* Add missing Amazon S3 yml to Makefile

* Document unstable instance

* Changes to be able to build container in Google Cloud

* Copy config examples when making build

* Add app to container and .dockerignore all else

* Fixed missed merge

* Add db configurability by env vars for containers

* Fix db config

* Copy configs

* Switch keyserver

* Add env vars, tweak make

* Substitute env vars parameters

* Env var control

* Show env vars

* env not ENV

* add DB_SOCKET

* Add recomended parameters

* Not deploy app engine, show cloudsql dir

* Omit list cloudsql dir

* Added correct image tag

* Add database parameters as env vars

* Support $PORT env var

* Using Node 12 and Yarn for Dockerfile.txt as well

* Changing Passenger's port on production env

* Setting local db for travis

* set .env PORT to $PORT

* Remove .env

* Compose environment variableZ fallback

* Revert all files under /app to versions in main

* Revert to main

* Delete unneeded files

* Remove extra files from rebase

* Add bundle install as build step

* Deleted not needed Dockerfile

* Missed RUN in Dockerfile

* Add precompile step

* Hardcode environment at build time

* Adding missing yaml and update bootsnap version

* Omit /app/tmp from volume

* Revert try to get precompile to work

* Clean up patch for merging

* List variables in app.yaml

* Tweak for jenkins

* Add .env for jenkins

* Fix PORT for jenkins/docker-compose

* Address PORT properly

* New form ports

* Enclose docker-compose ports in quotes

* ports yaml should be object not array

* Try different format for ports

* Try docker-compose format

* Redirect script for AppEngine

* Try to revertt to working condition for appengine

* Point PORT in Procfile

* Revert to known good config in appengine

* Tweak assets precompilation

* Restore PORT setting

* Add redirect to map /warps directory to legacy archive

* Add .env for jenkins/docker-compose

* Add hardcoded route to legacy warps

* Remove .env for appengine

* Satisfy appengine docker-compose

* Ignore app.yaml

* Ignore app.yaml secrets

* Satisfy Jenkins wihout hurting appengine hopefully

Co-authored-by: Sebastian Silva <[email protected]>
Co-authored-by: rarrunategu1 <[email protected]>
Co-authored-by: Jeffrey Warren <[email protected]>
Co-authored-by: Milo MacPhail <[email protected]>
Co-authored-by: Sonali Agrawal <[email protected]>
Co-authored-by: Ananya Agrawal <[email protected]>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sasha Boginsky <[email protected]>
Co-authored-by: Kaustubh Nair <[email protected]>
Co-authored-by: Divya Baid <[email protected]>
Co-authored-by: Gaurav Sachdeva <[email protected]>
Co-authored-by: Govind Jeevan <[email protected]>
Co-authored-by: Cess <[email protected]>
Co-authored-by: Stefanni <[email protected]>
Co-authored-by: hc-barker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants