Problem/Motivation

In #2768371: Make Diff Plugins property aware we added recursive labels to display better the hierarchy of the fields. The labels are now displayed in Italic and we should try to find a better way to display them, maybe Bold?

Proposed resolution

Discuss, create a patch.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

yongt9412 created an issue. See original summary.

johnchque’s picture

StatusFileSize
new42.24 KB

Should be better to display it non Italic?

miro_dietiker’s picture

Hm, now they look uniform. ;-)
It might be a good idea to make them strong as usual with labels as a start.

IMHO the diff lines have too much space in the background color.

Also, what i don't understand is, why the lines need those background colors.
Plus the red bold word inside the diff is irritating. It feels like something is wrong there.

So beside just labels, we should recheck nice solutions and adapt best practices or define "the perfect approach" on our own.. ;-)
Also, please open an issue to only investigate accessibility of the revision list / selection and later table / diff display.

johnchque’s picture

Status: Active » Needs review
StatusFileSize
new1.85 KB
new41.97 KB

We might use the github theme as default then? Here there is an example with some changes in the label:

And a patch. :)

Status: Needs review » Needs work

The last submitted patch, 4: define_how_to_display-2784381-4.patch, failed testing.

johnchque’s picture

Status: Needs work » Needs review

Random test fail.

miro_dietiker’s picture

StatusFileSize
new41.12 KB

Well that kind of naming does not make sense then. A default that is not selected by default. ;-P

I'm asking myself why we have those multiple displays at all... We want an awesome thing by default - not 3 half baked starting points.
Users can customise it using standard approaches such as custom CSS and template overrides...

Also, if we are talking about github as a "default" reference, please checkout a real diff and adjust the output to better follow it.

As i mentioned above, at least the spacing is at least way smaller.

johnchque’s picture

Changed a bit the code I was able to replicate the github look. this will for sure break tests. And because of this change there is a problem with the headers where they are not properly placed anymore because the diffs are not displayed by columns.

With this new patch the diff comparison looks like this:

Status: Needs review » Needs work

The last submitted patch, 8: define_how_to_display-2784381-8.patch, failed testing.

miro_dietiker’s picture

Priority: Normal » Critical

Interesting. ;-)

Now that you started with it... Would line numbers be possible?
Did you see, the line numbers and +/- also have colored background, intentionally slightly different.

At least it proofs that based on the display, we need different output layouts.
So for me, a feasible difference is not only CSS colors, it's much more appearance in general.

IMHO both displays could make sense.

So sorry for adding complexity: ;-)
Instead of selecting a theme, i would propose that we have plugins to display the diff and they can be enabled or disabled.
The button shows the enabled plugins, with the default one initially.

These plugins would make sense to me:
- Side by side diff, colored, with bold labels, like #4, but reduced vertical spacing
- Github style diff, with line numbers, single column, with bold labels
- HTML DOM tree based diff, newlines are ignored, bold labels + parent tag tree, tag children are indented
- Markdown, with bold labels, single or two col?
- Visual rendered document, no labels, single col

Formatting semantics could also be picked from per-content-instance field text format settings (markdown, HTML, ...) so they wouldn't need to be any specific layout selections...

johnchque’s picture

Status: Needs work » Postponed
miro_dietiker’s picture

Priority: Critical » Major
Status: Postponed » Needs work

So the layout plugin system is in, this was still postponed...

But i guess most of the items mentioned above we alread fixed? :-)
Can we even mark it fixed?

johnchque’s picture

Status: Needs work » Fixed

Yes, the changes proposed here were made already during other commits, so I think we can set it as fixed. :)

Status: Fixed » Closed (fixed)

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