Page MenuHomePhabricator

Padding on icons in VE mobile context broken by OOUI update
Closed, ResolvedPublic1 Estimated Story Points

Event Timeline

Esanders created this task.Apr 20 2017, 5:54 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 20 2017, 5:54 PM
Jdforrester-WMF triaged this task as High priority.Apr 20 2017, 6:01 PM
Jdforrester-WMF moved this task from To Triage to Epics on the VisualEditor board.
Jdforrester-WMF set the point value for this task to 1.
DLynch added a subscriber: DLynch.Apr 27 2017, 4:51 PM

Might be a side-effect of my changes to buttongroups in toolbars (d22d2331), somehow.

That said, is there an image somewhere of what this is supposed to look like?

Ah, I see, it's more the spacing on the entire row than the icons themselves.

You broke it, etc. ;-)

DLynch added a comment.EditedApr 28 2017, 11:10 PM

I think it's actually maybe Volker! There were several commits in the last release cycle or two which involved refactoring padding on ButtonElement, particularly 99361c772c which had some impact on non-labelElement buttons. It's enough of a pain to force in a different version of OOui that I haven't tested this, but it looks more plausible.

Anyway, working on a fix now.

Change 350962 had a related patch set uploaded (by DLynch; owner: DLynch):
[VisualEditor/VisualEditor@master] MobileContext: fix button spacing

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

Yup, I haven't touched toolbar tools (yet), but as there are probably a few ButtonElements scattered here and there my changes might have been part of the cause. Going to help with reviewing…

My patch applies some custom padding to non-labelWidget buttons in the header of a few places in VE. I'm not sure if this would be better placed up in OOui itself; I can move it into a patch for that, if people think that's better.

From first reaction, I'd be with your comment – being inclined to have it centralized in OOUI as we've got a collection of toolbar styles and I don't see why we wouldn't want to have this generalized (without looking in all possible implementations).

Volker_E updated the task description. (Show Details)May 2 2017, 12:16 AM

Change 351341 had a related patch set uploaded (by DLynch; owner: DLynch):
[oojs/ui@master] ProcessDialog: safe action buttons were misaligned

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

Change 351353 had a related patch set uploaded (by Esanders; owner: Esanders):
[oojs/ui@master] Fix padding for frameless buttons in ProcessDialogs

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

Change 351341 abandoned by DLynch:
ProcessDialog: safe action buttons were misaligned

Reason:
Ed's patch

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

Change 351353 merged by jenkins-bot:
[oojs/ui@master] MediaWiki theme: Fix padding for frameless buttons in ProcessDialogs

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

Change 350962 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] MobileContext: fix button spacing

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

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

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

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

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

Jdforrester-WMF closed this task as Resolved.May 10 2017, 3:40 PM
Jdforrester-WMF removed a project: Patch-For-Review.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMay 10 2017, 3:40 PM