Page MenuHomePhabricator

Only show 15px of left margin from historical visual diffs when the diff contains a move
Closed, ResolvedPublic1 Story Points

Description


Any reason the visualdiff content adds 15px of left margin ? It's a bit jarring for historical diffs, as it messes with the alignment in the view, which gives it a more 'busy' look then needed.

.ve-ui-diffElement-content {
left-margin: 15px;
}

To my knowledge, there is no particular reason it is this way. I agree it should be changed.

Event Timeline

Deskana created this task.Feb 6 2018, 7:47 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 6 2018, 7:47 PM
Deskana triaged this task as Low priority.Feb 6 2018, 7:47 PM
Deskana moved this task from To Triage to TR6: Visual diffs on the VisualEditor board.
Deskana set the point value for this task to 1.
Esanders added a subscriber: Esanders.EditedFeb 7 2018, 10:51 AM

The padding is for showing the move markers:

In the historical diffs (and on Vector) there is already a margin, so we could apply a negative margin to the whole element.

Removing the margin would be a problem in Monobook though:

These left margin markers may not exist after the next design iteration though (T169325)

We could also track if there were any moves at all and only show the margin in those cases, similar to what we do with the right sidebar.

Change 408779 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] DiffElement: Only show left margin if moves present

https://gerrit.wikimedia.org/r/408779

TheDJ added a comment.Feb 7 2018, 4:05 PM

I like that solution Ed ! nice work.

Change 408779 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] DiffElement: Only show left margin if moves present

https://gerrit.wikimedia.org/r/408779

Deskana renamed this task from Remove added 15px of left margin from historical visual diffs to Only show 15px of left margin from historical visual diffs when the diff contains a move.Feb 8 2018, 2:20 PM
Deskana closed this task as Resolved.
Deskana assigned this task to Esanders.

Thanks for the feedback, @TheDJ!

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptFeb 8 2018, 2:21 PM

Change 409632 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (c39a4a69b)

https://gerrit.wikimedia.org/r/409632

Change 409632 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (c39a4a69b)

https://gerrit.wikimedia.org/r/409632