[Launchpad-reviewers] [Merge] ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master
The proposal to merge ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master has been updated. Status: Approved => Merged For more details, see: 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. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master
The proposal to merge ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master has been updated. Status: Needs review => Approved For more details, see: 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. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master
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")
Got it, thanks.
> 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());
Mm. There's quite a lot of code to arrange for relative dates, so I think if
we were doing that it would make sense to first explore getting the webapp to
render it rather than effectively duplicating that code into JavaScript.
> +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
Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master
Pushed the requested changes. This should be good to go now.
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")
The displaying of the pencil icon is already guarded by
`tal:condition="view/can_edit"`.
We need to add `editable-message` class on every message so we can add the link
to the revision history list. Everyone is able to see such list, regardless on
having or not permission to edit the message itself.
> 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());
It makes sense. I'll use toLocaleString here.
I thought also about implementing something in JS to display it as a relative
date (like 'x days ago', 'y months ago', etc), but I guess this can go in
another MP.
> +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
> +};
> +
Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master
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.
> +
[Launchpad-reviewers] [Merge] ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master
The proposal to merge ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master has been updated. Description changed to: Some screenshots: - https://monosnap.com/file/au1QEPw7qb40DUMWaAUOE6c3N4QEGU - https://monosnap.com/file/ENRLcSbY51NObjAAq3aJHH8WEm2tg4 - https://monosnap.com/file/PG2bG2UCBqn6BuOVlbffpeXWkwuW33 For more details, see: https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/403213 -- Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] ~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master
Thiago F. Pappacena has proposed merging
~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master.
Commit message:
Showing message revision list in the UI
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/403213
--
Your team Launchpad code reviewers is requested to review the proposed merge of
~pappacena/launchpad:comment-editing-ui-list-revisions into launchpad:master.
diff --git a/lib/canonical/launchpad/icing/css/base.scss b/lib/canonical/launchpad/icing/css/base.scss
index 6ee2779..1e33ca7 100644
--- a/lib/canonical/launchpad/icing/css/base.scss
+++ b/lib/canonical/launchpad/icing/css/base.scss
@@ -579,6 +579,61 @@ body {
margin: 5px;
}
}
+
+.message-revision-container {
+position: absolute;
+background-color: white;
+margin-top: 20px;
+display: none;
+border: 1px solid #ddd;
+width: 45em;
+z-index: 100;
+max-height: 200px;
+overflow-y: auto;
+overflow-x: hidden;
+
+.message-revision-container-header {
+background-color: #eee;
+border-bottom: 1px solid #ddd;
+padding: 10px;
+
+span {
+text-align: left;
+display: inline-block;
+font-size: 110%;
+font-weight: bold;
+}
+
+img {
+float: right;
+cursor: pointer;
+}
+}
+
+.message-revision-item {
+border-bottom: 1px solid #ddd;
+padding: 2px;
+
+.message-revision-title {
+padding: 5px;
+cursor: pointer;
+font-weight: 300;
+}
+
+.message-revision-body {
+display: none;
+padding-left: 20px;
+padding-bottom: 10px;
+}
+}
+
+.active {
+background-color: #eee;
+.message-revision-title {
+font-weight: bold;
+}
+}
+}
}
@import 'typography',
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")
return " ".join(css_classes)
@property
diff --git a/lib/lp/answers/stories/question-workflow.txt b/lib/lp/answers/stories/question-workflow.txt
index 5e39acd..1a0b67a 100755
--- a/lib/lp/answers/stories/question-workflow.txt
+++ b/lib/lp/answers/stories/question-workflow.txt
@@ -215,7 +215,7 @@ The confirmed answer is also highlighted.
>>> soup = find_main_content(owner_browser.contents)
>>> bestAnswer = soup.find_all('div', 'boardComment')[-2]
->>> print(bestAnswer.find('img'))
+>>> print(bestAnswer.find_all('img')[1])
>>> print(soup.find(
diff --git a/lib/lp/answers/templates/questionmessage-display.pt b/lib/lp/answers/templates/questionmessage-display.pt
index fc3651c..c1ed29a 100644
--- a/lib/lp/answers/templates/questionmessage-display.pt
+++ b/lib/lp/answers/templates/questionmessage-display.pt
@@ -14,6 +14,25 @@
+
+
+ Revision history for this message
+
+
+
+
+
+
+
+
@@ -28,11 +47,12 @@
datetime context/datecreated/fmt:isodate"
tal:content="context/datecreated/fmt:displaydate">Thursday 13:21
-(last edit (last edit ):
+tal:content="context/date_last_edited/fmt:displaydate" />):
diff --git a/lib/lp/services/messages/interfaces/messagerevision.py b/lib/lp/services/messages/interfaces/messagerevision.py
index 3ee5a90..aa0ff0a 100644
--- a/lib/lp/services/messages/interfaces/messagerevision.py
+++ b/lib/lp/services/messages/interfaces/messagerevision.py
@@ -35,7 +35,8 @@ class IMessageRevisionView(Interface):
