Problem/Motivation

When comparing revision with the Unified fields layout the header of the comparison table shows only the revision link of the first revision, this can lead to misunderstanding of what is being compared. Fix it.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#13 interdiff-2818605-11-13.txt1.88 KBjohnchque
#13 improve_header_of-2818605-13.patch14.55 KBjohnchque
#11 interdiff-2818605-8-11.txt4.77 KBjohnchque
#11 improve_header_of-2818605-11.patch13.57 KBjohnchque
#8 improve_header_of-2818605-8.patch13.22 KBjohnchque
#2 improve_header_of-2818605-2.patch2.35 KBjohnchque
#2 Screenshot from 2016-10-14 12-14-29.png39.57 KBjohnchque
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yongt9412 created an issue. See original summary.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Active » Needs review
FileSize
Screenshot from 2016-10-14 12-14-29.png39.57 KB
improve_header_of-2818605-2.patch2.35 KB

This should make it better. This will make the title follow the width of the rows below.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Needs review » Needs work 
+++ b/src/Plugin/diff/Layout/UnifiedFieldsDiffLayout.php
@@ -243,16 +243,29 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+          '#prefix' => '<div>',
+          '#suffix' => '</div>',
...
+          '#prefix' => '<span class="diff-header-right-prefix"></span><span>',
+          '#suffix' => '</span>',

Once span, once div?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented

Also not sure about that indentation. I thought we do two header lines.
Indentation will be wrong if the lines count is >9 or >99 which makes the col wider.

BTW how will it look with a message?

lightguardjp’s picture

lightguardjp CreditAttribution: lightguardjp commented

Wouldn't it make more sense to make it more like diff and use - and + for the different versions in the header?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented

@lightguardjp I don't get your proposal.
Both sides - the first and the second revision - can provide + and - lines. There's nothing to indicate like that in the header.

"More like diff" - please provide a screenshot if there is something existing we can adapt.

We could discuss to pick up the header from visual inline diff plugin and remove the comment from the header.
But if we would do so, we should also discuss to remove (comment and author) also in the split field diff header.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented

@miro_dietiker by now, the header in Unified layout doesn't show the revision log.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
improve_header_of-2818605-8.patch13.22 KB

Started from 0, now we use the same method as the visual inline layout. :)

Status: Needs review » Needs work

The last submitted patch, 8: improve_header_of-2818605-8.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented 
  1. +++ b/src/DiffLayoutBase.php
@@ -104,6 +104,62 @@ abstract class DiffLayoutBase extends PluginBase implements DiffLayoutInterface,
+      $revision_link = $this->t('@date', [

    t() is to translate. "@date" is not a translatable string, thus that's an illegal usage. 

  2. +++ b/src/DiffLayoutBase.php
@@ -112,14 +168,38 @@ abstract class DiffLayoutBase extends PluginBase implements DiffLayoutInterface,
-      $revision_link = $this->t($revision_log . '@date', [

    Oh wow, that was wrong too. a t() should only operate on a static string.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
improve_header_of-2818605-11.patch13.57 KB
interdiff-2818605-8-11.txt4.77 KB

Changes things based on comment above, fixed type, improved css. This should work.

Status: Needs review » Needs work

The last submitted patch, 11: improve_header_of-2818605-11.patch, failed testing.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
improve_header_of-2818605-13.patch14.55 KB
interdiff-2818605-11-13.txt1.88 KB

This time, tests are also fixed.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Needs review » Fixed

This looks very nice.
Only the navigation (50/50, centered) still looks a bit lost now.

Now that the sidebar looks like really hard to achieve, we can reduce the header a bit with navigation floating.
And then i think we have a great UX to close the v1.

Status: Fixed » Closed (fixed)

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