launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/answers/browser/question.py b/lib/lp/answers/browser/question.py
> index 9d4986d..52cff40 100644
> --- a/lib/lp/answers/browser/question.py
> +++ b/lib/lp/answers/browser/question.py
> @@ -1195,13 +1195,11 @@ class QuestionMessageDisplayView(LaunchpadView):
>          return check_permission('launchpad.Moderate', self.context)
>  
>      def getBoardCommentCSSClass(self):
> -        css_classes = ["boardComment"]
> +        css_classes = ["boardComment", "editable-message"]
>          if not self.context.visible:
>              # If a comment that isn't visible is being rendered, it's being
>              # rendered for an admin or registry_expert.
>              css_classes.append("adminHiddenComment")
> -        if self.can_edit:
> -            css_classes.append("editable-message")

This seems a bit odd.  Was it intentional?  It seems surprising to end up with the editable-message class on a message where you don't have edit access.

>          return " ".join(css_classes)
>  
>      @property
> diff --git a/lib/lp/services/messages/javascript/messages.edit.js b/lib/lp/services/messages/javascript/messages.edit.js
> index 3c4c0e5..912a24c 100644
> --- a/lib/lp/services/messages/javascript/messages.edit.js
> +++ b/lib/lp/services/messages/javascript/messages.edit.js
> @@ -168,6 +179,74 @@ YUI.add('lp.services.messages.edit', function(Y) {
>          elements.update_btn.getDOMNode().disabled = false;
>      };
>  
> +    module.fillMessageRevisions = function(elements, revisions) {
> +        // Position the message revision list element.
> +        revisions = revisions.reverse();
> +        var revisions_container = elements.container.one(
> +            ".message-revision-container");
> +        var last_edit_el = elements.last_edit.getDOMNode();
> +        var target_position = last_edit_el.getBoundingClientRect();
> +        var nodes_holder = revisions_container.one(".message-revision-list");
> +        var template = revisions_container.one(
> +            "script[type='text/template']").getDOMNode().innerHTML;
> +
> +        revisions_container.setStyle('left', target_position.left);
> +        revisions_container.setStyle('display', 'block');
> +        revisions_container.one(".message-revision-close").on(
> +            "click", function() {
> +                nodes_holder.getDOMNode().innerHTML = '';
> +                revisions_container.setStyle('display', 'none');
> +            });
> +
> +        var content = "";
> +        revisions.forEach(function(rev) {
> +            var attrs = rev.getAttrs();
> +            var date_created = new Date(attrs.date_created);
> +            attrs.date_created = (
> +                date_created.toISOString().substr(0,10) + ", " +
> +                date_created.toLocaleTimeString());

Since `toISOString` always uses UTC, this seems likely to be problematic if the local timezone offset causes the UTC time and the local time to be on different days.  Would it maybe be better to just use `toLocaleString` for the whole thing?  (Or we could have another attribute on the revision that causes the webapp to render `date_created` in the user's timezone, which would be more consistent with the rest of the UI but probably more effort.)

> +            attrs.content = module.htmlify_msg(attrs.content);
> +            content += Y.Lang.sub(template, attrs);
> +        });
> +
> +        nodes_holder.getDOMNode().innerHTML = content;
> +        nodes_holder.all(".message-revision-item").each(function(rev_item) {
> +            rev_item.one(".message-revision-title").on('click', function() {
> +                nodes_holder.all('.message-revision-body').setStyle(
> +                    'display', 'none');
> +                var body = rev_item.one('.message-revision-body');
> +                var current_display = body.getStyle('display');
> +                body.setStyle(
> +                    'display', current_display === 'block'? 'none' : 'block');
> +                nodes_holder.all(".message-revision-item").removeClass(
> +                    'active');
> +                rev_item.addClass('active');
> +            });
> +        });
> +    };
> +
> +    module.onLastEditClick = function(elements, baseurl) {
> +        // Hide all open revision containers.
> +        Y.all('.message-revision-container').each(function(container) {
> +            container.setStyle('display', 'none');
> +        });
> +
> +        var url = "/api/devel" + baseurl + "/revisions";
> +        var config = {
> +            on: {
> +                 success: function(response) {
> +                    module.fillMessageRevisions(elements, response.entries);
> +                 },
> +                 failure: function(err) {
> +                    alert("Error fetching revisions.");
> +                 }
> +             },
> +             // XXX pappacena 2021-05-19: Pagination will be needed here.
> +             size: 100
> +        };
> +        this.lp_client.get(url, config);
> +    };
> +
>      module.wireEventHandlers = function(container) {
>          var node = container.getDOMNode();
>          var baseurl = node.dataset.baseurl;


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/403213
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:comment-editing-ui-list-revisions.