Problem/Motivation

We recently added a revision log summary, now the revision log is displayed twice in the revision listing. Remove it from the Revision column.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

yongt9412 created an issue. See original summary.

johnchque’s picture

Status: Active » Needs review
StatusFileSize
new1.31 KB

Trivial patch.

miro_dietiker’s picture

Status: Needs review » Needs work

Generally agree, we need to get rid off the duplication.

But since the summary has the format of text, it needs space.
Instead of exposing it as a separate "Summary" column, i would add it at the previous location (below the date) instead.
Also, to guarantee uniqueness, we should assertUnique once in tests.

miro_dietiker’s picture

Issue tags: +Usability
johnchque’s picture

Ok, gonna fix that then.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new3.42 KB
new2.64 KB

Should be better now.

miro_dietiker’s picture

Status: Needs review » Needs work

We should have updated the issue summary and provided a screenshot as it is a UI change.

+++ b/src/Form/RevisionOverviewForm.php
@@ -215,13 +214,12 @@ class RevisionOverviewForm extends FormBase {
+                    '#markup' => $this->entityComparison->getRevisionDescription($revision, isset($vids[$key + 1]) ? $vids[$key + 1] : $vids[$key]),

@@ -284,13 +282,12 @@ class RevisionOverviewForm extends FormBase {
+                    '#markup' => $this->entityComparison->getRevisionDescription($revision, isset($vids[$key + 1]) ? $vids[$key + 1] : $vids[$key]),

So two confusions:
- The duplication: I realised the spaghetti code at RevisionOverviewForm is highly redundant and should and can be be unified.
- I don't like this key shifting for +1. And then i realised in RevisionOverviewForm, we are loading count($vids)-1 twice because we just pass in the previous revision id and then load it deep inside summary(). You try to delegate loading, but still need to maintain conditions if $key isn't existing. That's bad delegation. If it's the last revision, just don't pass in a second revision(_id) at all. But i think we should switch $previous_revision_id with $previous_revision and load early. Small API change although beta, but IMHO mostly internal.

Committing, back to needs work to create the issue follow-ups. Please close when issues created.

johnchque’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.