[aur-dev] [PATCH v2 3/3] pkg_comments.php: Add JavaScript function to edit comments

[Lukas Fleischer]

On Tue, 21 Jul 2015 at 17:59:34, Marcel Korpel wrote:
> Signed-off-by: Marcel Korpel <marcel.korpel at gmail.com>
> ---
> Changes since v1:
> * Drop the AJAX form submission, instead use a normal form.
> * Pass variables $base_id and $pkgbase_name that are now needed when
>   rendering the form.
> * Add loading indicator.
> 
>  web/html/css/aurweb.css         |   6 ++++++
>  web/html/images/ajax-loader.gif | Bin 0 -> 723 bytes
>  web/html/index.php              |   4 ++++
>  web/template/pkg_comments.php   |  30 ++++++++++++++++++++++++++++++
>  4 files changed, 40 insertions(+)
>  create mode 100644 web/html/images/ajax-loader.gif
> [...]
> diff --git a/web/template/pkg_comments.php b/web/template/pkg_comments.php
> index 26fddfd..eac0da1 100644
> --- a/web/template/pkg_comments.php
> +++ b/web/template/pkg_comments.php
> @@ -72,3 +72,33 @@ $count = pkgbase_comments_count($base_id, $include_deleted);
>         </h3>
>  </div>
>  <?php endif; ?>
> +<script>
> +$(document).ready(function() {
> +       $(".edit-comment").click(function () {
> +               var parent_element = this.parentElement,
> +                       edit_form = $(parent_element).next(),
> +                       comment_id = parent_element.id.substr(8),

Hardcoding the prefix length here seems error prone. How about splitting
after the first hyphen instead?

> +                       _this = $(this);

Do we need an extra variable here?

> +               add_busy_indicator(_this);
> +               $.getJSON('<?= get_uri('/rpc') ?>',
> +                       {type: 'load-comment',
> +                        arg: comment_id,
> +                        base_id: <?= htmlspecialchars($base_id) ?>,

We should use intval() instead of htmlspecialchars() here.

> +                        pkgbase_name: '<?= htmlspecialchars($pkgbase_name) ?>'},

Can we use htmlspecialchars() here? If it is the right function to use,
we need to add ENT_QUOTES at least...

> +                       function (data) {

Broken indentation?

> +                               remove_busy_indicator(_this);
> +                               edit_form.html(data.form);
> +                               edit_form.find('textarea').focus();
> +               });

Broken indentation (closing curly brace not aligned with the function
keyword)?

> +               return false;
> +       });
> +
> +       function add_busy_indicator(sibling) {
> +               sibling.after('<img src="/images/ajax-loader.gif" class="ajax-loader" width="16" height="11" alt="Busy…">');

Do we need to close that tag? Like "<img [...] />"?

Thanks!

> +       }
> +
> +       function remove_busy_indicator(sibling) {
> +               sibling.next().remove();
> +       }
> +});
> +</script>
> -- 
> 2.4.6