Page MenuHomePhabricator

Visual diff change colors hidden by <code> tag
Closed, ResolvedPublic

Description

This diff shows up visually like this:

visual preview code color.png (615×903 px, 132 KB)

Note how the word GENewcomerTasksConfigTitle seems unchanged, even though it was added now. <code> / <tt> inside the change-color span should have its own colored styling.

Event Timeline

I got the essence of this issue. Do I have to setup it locally and push it or edit it using Gerrit web interface ( as only few lines of CSS will do the desired thing )?
Please guide me.

image.png (463×1 px, 48 KB)

To distinguish <code> tag from text, I have used light shades of the same colors both for ins[data-diff-action='insert'] and del[data-diff-action='remove']. Is it correct? @Tgr @matmarex

I'd just use the same color. Lighter green/red often has a special meaning in diffs to highlight text that changed in some indirect way (e.g. the unchanged part of a line that changed) so it might be confusing, and the presence of <code> can be seen from the font (harder to spot than the normal background color, but it's not really relevant information for the diff, anyway).

Sure, I will use the same colors. Can I edit these changes directly using Gerrit web interface (as only few lines of CSS will do the desired thing)? @Tgr @matmarex

Change 676786 had a related patch set uploaded (by A. Amritesh; author: A. Amritesh):

[VisualEditor/VisualEditor@master] DiffElement: Add CSS for <code> tag

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

image.png (1×1 px, 303 KB)

What to do, -2 is given to patch? I have made changes to the mentioned directory.
Please guide me. @Tgr @matmarex @Aklapper

Should be fixed somewhere in https://phabricator.wikimedia.org/diffusion/GVED/browse/master/src/ui/styles/elements/ve.ui.DiffElement.css, there are a few other overrides already for images and tables.

@A.Amritesh: (Please don't ping for no good reason. Thanks. Please also don't ping both about patches here in Phab and in Gerrit. Thanks.) The review comment says that the code change should happen in another code repository. That means a separate new patch is needed (as you can only amend patches in the very same repo).

There are two demos, I will do changes according which you select : @Esanders @Tgr
Demo 1

image.png (432×1 px, 52 KB)

Demo 2
image.png (431×1 px, 51 KB)

Thanks for these @A.Amritesh. I’m happy with Demo 1, and it results in cleaner CSS.

What to do, -2 is given to patch? I have made changes to the mentioned directory.

Sometimes you will be given guidance by one developer, but another developer will come in with a different opinion. Hopefully this won’t happen too often, and apologies for any confusion this caused.

I initially gave a -2 because I was proposing moving the code to another repository, hence “do not merge this” was the most appropriate instruction (even though the patch was heading in the right direction). @Tgr then pointed out the code could in fact go in that repo, so I updated to -1 as I still had some changes I wanted to propose.

I have updated the code and commit message. Please review it. Thanks for your guidance and support as it was my first time contribution in MediaWiki. @Esanders @Tgr

Change 676786 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] DiffElement: Add CSS for <code> tag

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

@A.Amritesh Sorry I didn't reply to your messages earlier, I get lots of notifications. I'm glad you worked it out!

Change 677670 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (83f89605d)

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

Change 677670 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (83f89605d)

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

Thanks :) for all your guidance and support again, finally my first contribution got merged.