Page MenuHomePhabricator

[Bug] Fix hover area and placement on settings cog in Page Preview panel
Closed, ResolvedPublic3 Story Points

Description

Steps to reproduce

Hover over a settings cog in a page preview panel, look at the gray area that appears

Hover area

The gray area that appears when you hover should be:
-square
-equal 7px around icon on all sides

Placement

The settings cog should have equal distance from the right and bottom of the page preview panel
-27px from right
-27px from bottom

Acceptance criteria

  • remove .mwe-popups .icon rule
  • use flexbox
  • Use appropriate/consistent use of mw-ui-icon (label inside icon)

See https://phabricator.wikimedia.org/T193058#4163023 for more details

  • Update icon asset

  • Ensure design is happy before merging.

Testing

We should make sure to test on both horizontal and vertical, RTL and LTR

Details

Related Gerrit Patches:
mediawiki/extensions/Popups : masterAdjusts margin on settings icon
mediawiki/extensions/Popups : masterUpdate setting icon

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 25 2018, 5:13 PM
alexhollender triaged this task as Medium priority.Apr 25 2018, 6:05 PM

I think the margins are not the problem here. All our icons are like this. Changing these is not as straightforward as you'd think and carries a lot of risk so let's not do that.


I'm not sure if this is a problem, but we seem to be adding a margin to the settings icon - is this the appropriate positioning?

Removing that gives us the correct positioning (am I right? (if not lemme know and I won't touch that):

Then the next issue is the gray background focus when clicked.

I think we can fix this by making the icon (note the "Disable settings" text added for accessibility)

<a class="mwe-popups-settings-icon" href="/wiki/Special:Preferences#mw-prefsection-rendering">
  <div class="mw-ui-icon mw-ui-icon-element mw-ui-icon-popups-settings">Disable settings</div>
</a>

and a bit of flexbox:

.mwe-popups .mwe-popups-container footer .mwe-popups-settings-icon {
    float: right;
    width: 40px;
    height: 40px;
    background: #ccc;
    display: flex;
    align-items: center;
    justify-content: center;
}

Does this look like the right effect?:

If so I think I can add some notes to the description and this is ready to go.

@Jdlrobson right, good point that this is happening with all icons (although I think you only notice it on mobile, whereas this is on desktop). Will make a note to follow up about that comprehensively.

In terms of your screenshot above, yes it looks like it's moving in the right direction. The padding is still unequal, and the placement is off, but I think you know that :)

Jdlrobson updated the task description. (Show Details)May 1 2018, 8:04 PM
Jdlrobson updated the task description. (Show Details)

The padding is still unequal, and the placement is off, but I think you know that :)

Not sure I do! Where are you seeing the issue? Is it possible that the settings icon we are using is off? When I look at it is doesn't look centered:
https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/resources/ext.popups.images/cog.svg
I guess the icon needs to be updated too?

@Jdlrobson ah yea, that seems like part of the issue. I've attached the proper icon from OOUI here.

Also, re: padding and placement, check out the details in the task description.

Padding: equal 7px around icon on all sides
Placement: 27px from right & bottom

Jdlrobson updated the task description. (Show Details)May 1 2018, 9:08 PM
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.
ovasileva set the point value for this task to 3.May 22 2018, 4:36 PM

Change 435978 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/Popups@master] Update setting icon

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

Change 435978 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Update setting icon

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

This is up on http://reading-web-staging.wmflabs.org/ for you to design review. Is the settings cog correctly placed? After that, I think this can go to Anthony for a general page previews test of the disabling/enabling page previews scenarios.

@Jdlrobson looks like it needs to come down 2px. Otherwise looking good.

Change 439775 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/Popups@master] Adjusts margin on settings icon

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

Jdlrobson added a subscriber: Jdrewniak.

This is on the beta cluster and staging for another round of design review. Please wait 30 mins from this comment before testing.

Change 439775 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Adjusts margin on settings icon

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

👍 thinking this can skip QA and go straight to sign-off

ovasileva closed this task as Resolved.Jun 12 2018, 3:43 PM

all done!

Vvjjkkii renamed this task from [Bug] Fix hover area and placement on settings cog in Page Preview panel to l8daaaaaaa.Jul 1 2018, 1:13 AM
Vvjjkkii reopened this task as Open.
Vvjjkkii removed ovasileva as the assignee of this task.
Vvjjkkii raised the priority of this task from Medium to High.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed the point value for this task.
Vvjjkkii edited subscribers, added: ovasileva; removed: gerritbot, Aklapper.
Mainframe98 renamed this task from l8daaaaaaa to [Bug] Fix hover area and placement on settings cog in Page Preview panel.Jul 1 2018, 10:00 AM
Mainframe98 closed this task as Resolved.
Mainframe98 assigned this task to ovasileva.
Mainframe98 lowered the priority of this task from High to Medium.
Mainframe98 updated the task description. (Show Details)
Mainframe98 set the point value for this task to 3.
Mainframe98 edited subscribers, added: gerritbot, Aklapper; removed: ovasileva.