Page MenuHomePhabricator

Replace custom pin icon with OOUI's 'pushPin'
Closed, ResolvedPublic

Description

Since v0.21.3 OOUI features a 'pushPin' icon, RevisionSlider should make use of this standard icon.

It should be placed in a OOUI ToggleButtonWidget (example)

Adjustments needed:

  • The style should be frameless, something that probably needs to be solved by CSS (there is a frameless button, but it is not a toggle-button)
  • It seems that the icon is too big in standard configuration now, It should have (roughly) the same size as now: 26*26px for the button area, 16*16 for the pushpin.

Helpful Links:

https://doc.wikimedia.org/mediawiki-core/master/js/#!/api/OO.ui.ToggleButtonWidget
https://www.mediawiki.org/wiki/Manual:Extension.json/Schema
https://phabricator.wikimedia.org/source/mediawiki/browse/master/docs/extension.schema.v1.json

Info for the Hackathon 2018
If you are interested in working on this at the Wikimedia-Hackathon-2018 , @WMDE-Fisch can help you with any questions :-)

Details

Related Gerrit Patches:
mediawiki/extensions/RevisionSlider : masterReplace custom 'pin' by standard library 'pushpin' icon
mediawiki/extensions/RevisionSlider : masterReplace custom 'pin' by 'pushPin' icon
mediawiki/extensions/RevisionSlider : masterReplace custom 'pin' by 'pushpin' icon
mediawiki/extensions/RevisionSlider : masterRevisionSlider: Fix missing pin icon

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Bmueller updated the task description. (Show Details)

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.

thiemowmde moved this task from Incoming to Revision Slider on the TCB-Team board.Jun 12 2018, 4:16 PM
Vvjjkkii renamed this task from Replace custom pin icon with OOUI's 'pushPin' to e1caaaaaaa.Jul 1 2018, 1:10 AM
Vvjjkkii raised the priority of this task from Medium to High.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from e1caaaaaaa to Replace custom pin icon with OOUI's 'pushPin'.Jul 2 2018, 5:38 AM
CommunityTechBot lowered the priority of this task from High to Medium.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.
Jan_Dittrich updated the task description. (Show Details)Jul 11 2018, 3:51 PM
Jan_Dittrich updated the task description. (Show Details)Jul 11 2018, 4:06 PM

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)

Gopavasanth added a comment.EditedJul 21 2018, 12:10 PM

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 :)

Gopavasanth renamed this task from Replace custom pin icon with OOUI's 'pushPin' to Add custom pin icon with OOUI's 'pushPin'.Jul 22 2018, 5:44 AM

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.

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

Gopavasanth renamed this task from Add custom pin icon with OOUI's 'pushPin' to Replace custom pin icon with OOUI's 'pushPin'.Jul 24 2018, 1:20 PM

@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


But still if this change is ok for now we can go with this or should we change this icon to use OOUI's icon now?
If needed I will try to change to use OOUI's Icon.

@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?

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.

thiemowmde added a comment.EditedAug 13 2018, 7:58 AM

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.
Gopavasanth added a comment.EditedAug 13 2018, 8:56 AM

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.

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?

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.

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:


New:

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.

@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

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.

@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.

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

Lea_WMDE closed this task as Resolved.Sep 5 2018, 3:52 PM
Lea_WMDE moved this task from Demo to Done on the Season of RevisionSlider board.

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

Reason:
Accomplished in Ifdaa2567c67a45fd44bea7cc4718f29c4e1a58fd

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