Page MenuHomePhabricator

"Add discussion" button on mobile has no margin, and is not centred on wider screens
Closed, ResolvedPublic

Description

image.png (630×785 px, 59 KB)

Event Timeline

Esanders added a subscriber: Jdlrobson.

Looks like a regression caused by a change in mw-ui-button selector specificity.

Esanders renamed this task from "Add discussion" button no mobile is not centred on wider screens to "Add discussion" button on mobile has not margin, and is not centred on wider screens.Oct 25 2021, 6:08 PM

Change 734369 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/skins/MinervaNeue@master] Increase specificity of talk button selector

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

The patch above fixes the talk button in this task, but there are other selectors in that file that might need looking for other regressions.

Jdlrobson added a subscriber: alexhollender_WMF.

Is the issue that this is not centered or that it's not full width?

The max width was an intentional change to bring us in line with the style guide which says that buttons have a max width. This button looks a bit odd when full width on desktop for example.

Why is this button special? If we remove the special casing it looks like:

Screen Shot 2021-10-25 at 12.43.47 PM.png (738×1 px, 83 KB)

The above patch makes it look like this which seems far worse:

Screen Shot 2021-10-25 at 12.44.28 PM.png (878×2 px, 132 KB)

cc @alexhollender

I think if you are going to set a max-width you need to update the styles of the buttons that have been specifically set and designed to be 100% width, that may include centring.

Given that other styles were overridden by the upstream change (e.g. the missing margins), I think these styles need to be audited, but in the meantime should revert to how they were originally intended (in T230695).

Esanders renamed this task from "Add discussion" button on mobile has not margin, and is not centred on wider screens to "Add discussion" button on mobile has no margin, and is not centred on wider screens.Oct 26 2021, 1:32 PM

to be honest I'm not sure I ever considered the design of the button/page on larger screens. I think the full-width button on mobile was meant to emphasize the action, but looking at it again (and considering larger screens) maybe the most simple thing is to remove the special width treatment? We could maybe add an icon to the button if we wanted to emphasize it more?

mobilewider
Screen Shot 2021-10-26 at 11.06.27 AM.png (687×396 px, 66 KB)
Screen Shot 2021-10-26 at 11.06.50 AM.png (777×987 px, 120 KB)

in any case I hope it's fair to consider this @iamjessklein's decision going forward

I'm not particularly concerned about larger screens at the moment, as desktop minerva is an edge case. I think we should merge the patch above to fix the regression, and consider the button width later. We may end up using a completely different design/location for the button anyway.

. I think we should merge the patch above to fix the regression

This is not a regression.

On mobile there is no visual regression:

Screen Shot 2021-10-26 at 8.51.55 AM.png (1×898 px, 151 KB)

As @alexhollender points out we never considered tablet / desktop in this original design.

While the UI has changed here,
In T283757 and https://gerrit.wikimedia.org/r/c/715991 we intentionally updated the button spec to match the current design guidelines. One of those guidelines is that there should be a max-width on the buttons, so this would be an expected consequence of those changes.

IMO the Minerva specific CSS code here should be removed in favor of the current design specification and if the design specification is wrong, please change it.

That patch also changes the button to add margin:0 with a specificity of 2. That seems problematic as now any button that wants to sent a margin has to use two selectors, causing breakages like this.

Why does the button class need to set the margin?

Why does the button class need to set the margin?

It probably doesn't need to. I think it's probably fine (and a good idea) to remove.

Change 734770 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/core@master] Follow-up I86568863: mw-ui-button: Reduce specificity of margin rule

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

Change 734770 merged by jenkins-bot:

[mediawiki/core@master] Follow-up I86568863: mw-ui-button: Reduce specificity of margin rule

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

Change 734369 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Remove custom styling from 'Add discussion' button

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

Test wiki created on Patch Demo by JKlein (WMF) using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/ca38119cc5/w/

Test wiki on Patch Demo by JKlein (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/ca38119cc5/w/

ppelberg added a subscriber: ppelberg.

We may end up using a completely different design/location for the button anyway.

Note: I've made a note to remind us to revisit how and where the Add discussion affordance appears here: T295101#7518017.