Problem/Motivation

Follow-up from #2784685: Streamline table header / row in GenericRevisionController

Header and row structure is still a bit complex and it's sometimes unclear what column is what.

Proposed resolution

To improve readability, we could use a col key in header and rows.
The keys are not used by the table element, but it still improves extensibiltiy and alterability as accessing cols is named.

CommentFileSizeAuthor
#11 interdiff-2785857-9-11.txt2.68 KBdrobnjak
#11 introduce_header_row-2785857-11.patch7.3 KBdrobnjak
#9 interdiff-2785857-7-9.txt3.24 KBdrobnjak
#9 introduce_header_row-2785857-9.patch7.3 KBdrobnjak
#9 Screenshot from 2016-10-03 16-09-24.png36.21 KBdrobnjak
#9 Screenshot from 2016-10-03 16-09-39.png35.9 KBdrobnjak
#7 introduce_header_row-2785857-7.patch3.94 KBjohnchque
#3 introduce_header_row-2785857-3.patch4.44 KBginovski

Comments

miro_dietiker created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

IMHO this could still help with making the conditional columns more clear (in what is skipped, what is kept, in header VS content column consistency).

ginovski’s picture

ginovski commented
Assigned: Unassigned » ginovski
Status: Active » Needs work
StatusFileSize
new introduce_header_row-2785857-3.patch4.44 KB

Initial patch for Unified fields layout.
1. Added keys to rows.
2. Removed row counter, since it is no longer needed.
3. Changed the if-condition array_slice with foreach unsetting the line-numbers at the end for the strip_tags filter.

@drobnjak will continue on this for Split fields layout.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs review » Needs work 
+++ b/src/DiffLayoutManager.php
@@ -106,7 +106,7 @@ class DiffLayoutManager extends DefaultPluginManager {
-    if (!$has_htmlDiffAdvanced) {
+    if ($has_htmlDiffAdvanced) {

Unrelated change.

The last submitted patch, 3: introduce_header_row-2785857-3.patch, failed testing.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
StatusFileSize
new introduce_header_row-2785857-7.patch3.94 KB

Just want to know if this breaks any test.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

Yeah, nice step.
Back to work to apply it to both plugins.

drobnjak’s picture

drobnjak commented
Assigned: ginovski » drobnjak
Status: Needs work » Needs review
StatusFileSize
new Screenshot from 2016-10-03 16-09-39.png35.9 KB
new Screenshot from 2016-10-03 16-09-24.png36.21 KB
new introduce_header_row-2785857-9.patch7.3 KB
new interdiff-2785857-7-9.txt3.24 KB

Changes applied also to Split fields layout. Screenshots of both layouts provided.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs review » Needs work 
  1. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
@@ -158,42 +158,32 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
+          'left-row-body' =>[
...
+          'right-row-body' =>[

+++ b/src/Plugin/diff/Layout/UnifiedFieldsDiffLayout.php
@@ -156,58 +155,48 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+            'row-body' => [
...
+              'row-body' => [

    lets change body by data.

Besides looks really nice.

drobnjak’s picture

drobnjak commented
Status: Needs work » Needs review
StatusFileSize
new introduce_header_row-2785857-11.patch7.3 KB
new interdiff-2785857-9-11.txt2.68 KB
johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs review » Reviewed & tested by the community

I like it really much.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Wonderful. Now even i can read it and follow the logic. ;-)

Status: Fixed » Closed (fixed)

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