You may want reduce the amount of OOUI overrides to prevent future regressions (and provide better skin support).

I would recommend having the pin and collapse buttons as simple icon-only buttons (right-aligned, instead of absolutely positioned), instead of the current nested buttons.

The ability to expand the toolbar by clicking on the label (or the space either side of it) can be re-added using a click handler to trigger the expand/collapse button, a bit how field labels can trigger checkboxes.

Seems reasonable but would clearly go for frameless buttons when updating this.

I would like to work on this task.

Hi @Gopavasanth, that is great to hear! Just assign yourself, and go ahead :) (You can assign if you choose "Assign/Claim" in the Add Action... dropdown)

Actually, There is no pin present right now,

So we have to place a new-OOUI pushPin button there right?
So can I modify the task heading from "Replace custom pin" to "Add push-Pin" ?

Oh wow, I just checked and you are right, the pin icon is gone! The link, however is still there. If you hover next to the arrow, you will get a tooltip, and if you click, a square will turn blue. If you want to, you can change the task title, but I would also be ok with keeping the current one. Your choice :)

Change 447221 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/RevisionSlider@master] RevisionSlider: Fix missing pin icon

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

Thanks for bringing this up!

@Lea_WMDE, can you please help splitting this into two separate tasks?

• The fact that the icon is gone is a bug. I uploaded https://gerrit.wikimedia.org/r/447221 to fix this. Please create a separate ticket for this, and make sure the fix gets backported to the live cluster.
• Please rename this task here back to "Replace …".

I see two options to proceed then:

• Either use the existing patch https://gerrit.wikimedia.org/r/432939 and bring it to an acceptable state. @Gopavasanth, you can continue working on existing patches, no need to be shy about this.
• If you follow Esanders suggestion (see T194613#4213027 above), it's probably easier to start with a fresh patch. But please make sure the bug that made the icon disappear is fixed first.

Sure Thanks @thiemowmde

Change 447445 had a related patch set uploaded (by Gopavasanth; owner: Gopavasanth):
[mediawiki/extensions/RevisionSlider@master] RevisionSlider: Fix missing pin icon

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

@Volker_E Can you please review https://gerrit.wikimedia.org/r/447445 So After fixing this we can move over to the real issue[Replacing the pin icon with OOUI's pushPin].

Change 447221 abandoned by WMDE-Fisch:
RevisionSlider: Fix missing pin icon

Reason:
Done in Ic24a175dfe8d19853e32669f12b9f17e26730d6b

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

Change 451611 had a related patch set uploaded (by Gopavasanth; owner: Gopavasanth):
[mediawiki/extensions/RevisionSlider@master] Replace custom 'pin' by 'pushpin' icon

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

Currently, with this patch https://gerrit.wikimedia.org/r/451611, the icon changed from pin to pushPin

@Gopavasanth From my point of view, it's preferable to directly make use of OOUI's icon and get rid of the SVG in RevSlider repo for easier maintainability.

Ok @Volker_E I am having a small doubt Currently, as it is in SVG when we pin the pin Icon it is changing it CSS properties and inverting its color but when we remove the SVG and replace with OOUI pushPin Icon and when we pin the OOUI pushPin Icon it won't change it's color properties as it's is OOUI icon, not the SVG. So what may your Idea If you use OOUI pushPin icon and If we pin the pushPin Icon? what are the expectations after pinning the pushPin Icon?

In T194613#4493642, @Gopavasanth wrote:

Ok @Volker_E I am having a small doubt Currently, as it is in SVG when we pin the pin Icon it is changing it CSS properties and inverting its color but when we remove the SVG and replace with OOUI pushPin Icon and when we pin the OOUI pushPin Icon it won't change it's color properties as it's is OOUI icon, not the SVG. So what may your Idea If you use OOUI pushPin icon and If we pin the pushPin Icon? what are the expectations after pinning the pushPin Icon?

So looking at your patch, that's not entirely true. When pinning the pushPin it behaves like with the old pin and turns blue - see my screenshot. I assume that's fine.

I reviewed patch set 6 of the patch https://gerrit.wikimedia.org/r/451611, and can confirm that it successfully replaces the icon as requested. But the patch does have issues, visible in this screenshot:

• The new icon is not centered on it's button.
• The slider UI below is partly shifted and appears broken with this patch. The vertical yellow bar belongs to the left side of the diff, but is shifted to the right. The blue bar does not connect to the circle any more. Never mind, this is most probably caused by my not up-to-date environment.

Yes, I agree @thiemowmde that the Icon is not centered and I fixed it now.

>The slider UI below is partly shifted and appears broken with this patch. The vertical yellow bar belongs to the left side of the diff, but is shifted to the right. The blue bar does not connect to the circle any more.

This is not happening in my local system and It's working good.

@Gopavasanth Are you using MediaWiki 1.30? (As I can see in your screenshot)

Note:- All the development work should be done on the master branch. and also Dependency (composer update) because OOUI is external Dependency

I am using Mediawiki 1.32.0-alpha (b8a29aa) I didn't rename the folder name.

In T194613#4497719, @thiemowmde wrote:

I reviewed patch set 6 of the patch https://gerrit.wikimedia.org/r/451611, and can confirm that it successfully replaces the icon as requested. But the patch does have two issues, both visible in this screenshot:

• The new icon is not centered on it's button.
• The slider UI below is partly shifted and appears broken with this patch. The vertical yellow bar belongs to the left side of the diff, but is shifted to the right. The blue bar does not connect to the circle any more.

Hmm strange - it also looks fine for me. - Is this with current core? Do you use gadgets? Does this happen on all diff pages? What browser/version is this happening on?

In T194613#4498033, @WMDE-Fisch wrote:

it also looks fine for me.

me too

I have the same MW version as @Gopavasanth.

Testing the patch right now.. though the push pin icon appears fine, I am not able to understand the significance of two icons elements here (push pin + expand). Instead, I'd expected only one icon to be present that changes as the container expands or collapses.

In T194613#4512272, @SrishtiSethi wrote:

I have the same MW version as @Gopavasanth.

Testing the patch right now.. though the push pin icon appears fine, I am not able to understand the significance of two icons elements here (push pin + expand). Instead, I'd expected only one icon to be present that changes as the container expands or collapses.

So the expand icon is to expand & collapse the slider ( that's easy ^^ ). With the pin, you're able to make the expanded state permanent. So the next time you're on a diff page the slider will be expanded automatically.

I think you always should be able to collapse the slider even if you have set the pin, so that's why the pin and the expand & collapse button need to be accessible at the same time.

@Charlie_WMDE @Lea_WMDE

From my side this patch looks ready. One question regarding the design though: By exchanging the icon of the pin the size of the symbol increases. Any opinions on that? Maybe make it roughly half the size?

Old:

The patch looks great to me, thanks for all the work! The one last remaining design question I leave to @Charlie_WMDE to answer as this is pure design :)

@WMDE-Fisch @Gopavasanth @Lea_WMDE

Are both the same size already? When you get the svg it has a "default" size with a frame. My question is, does one of them have a wider frame than the other or does the pin have a bigger height?

I would make them both the same size from the numbers, not visually if that makes sense.

In T194613#4544787, @Charlie_WMDE wrote:

@WMDE-Fisch @Gopavasanth @Lea_WMDE

Are both the same size already? When you get the svg it has a "default" size with a frame. My question is, does one of them have a wider frame than the other or does the pin have a bigger height?

I would make them both the same size from the numbers, not visually if that makes sense.

I just sat together with @Charlie_WMDE and confirmed, that the size in the patch fits.

Change 456383 had a related patch set uploaded (by Gopavasanth; owner: Gopavasanth):
[mediawiki/extensions/RevisionSlider@master] Merge commit 'refs/changes/11/451611/8' of https://gerrit.wikimedia.org/r/mediawiki/extensions/RevisionSlider into T194613

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

Change 456383 abandoned by Gopavasanth:
Merge commit 'refs/changes/11/451611/8' of https://gerrit.wikimedia.org/r/mediawiki/extensions/RevisionSlider into T194613

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

Change 456384 had a related patch set uploaded (by Gopavasanth; owner: Gopavasanth):
[mediawiki/extensions/RevisionSlider@master] Replace custom 'pin' by 'pushpin' icon

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

Change 456384 abandoned by Gopavasanth:
Replace custom 'pin' by 'pushpin' icon

Reason:
Sorry By mistake

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

Sorry for the above commits there is something going wrong in my system.

https://gerrit.wikimedia.org/r/456384
https://gerrit.wikimedia.org/r/456383

Change 451611 merged by jenkins-bot:
[mediawiki/extensions/RevisionSlider@master] Replace custom 'pin' by 'pushPin' icon

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

In T194613#4213027, @Esanders wrote:

You may want reduce the amount of OOUI overrides to prevent future regressions (and provide better skin support).

I would recommend having the pin and collapse buttons as simple icon-only buttons (right-aligned, instead of absolutely positioned), instead of the current nested buttons.

The ability to expand the toolbar by clicking on the label (or the space either side of it) can be re-added using a click handler to trigger the expand/collapse button, a bit how field labels can trigger checkboxes.

In T194613#4213085, @Volker_E wrote:

Seems reasonable but would clearly go for frameless buttons when updating this.

@Volker_E: The problem is that the push pin is a ToggleWidget (ToggleButtonWidget) which doesn't have a frameless mode. Currently it is implemented by overriding the CSS, but I think sticking to standard widgets would be preferable.

In T194613#4546052, @Esanders wrote:

In T194613#4213027, @Esanders wrote:

You may want reduce the amount of OOUI overrides to prevent future regressions (and provide better skin support).

I would recommend having the pin and collapse buttons as simple icon-only buttons (right-aligned, instead of absolutely positioned), instead of the current nested buttons.

The ability to expand the toolbar by clicking on the label (or the space either side of it) can be re-added using a click handler to trigger the expand/collapse button, a bit how field labels can trigger checkboxes.

In T194613#4213085, @Volker_E wrote:

Seems reasonable but would clearly go for frameless buttons when updating this.

@Volker_E: The problem is that the push pin is a ToggleWidget (ToggleButtonWidget) which doesn't have a frameless mode. Currently it is implemented by overriding the CSS, but I think sticking to standard widgets would be preferable.

I created an extra ticket for this discussion: T203553

Change 432939 abandoned by VolkerE:
Replace custom 'pin' by standard library 'pushpin' icon

Reason:
Accomplished in Ifdaa2567c67a45fd44bea7cc4718f29c4e1a58fd

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