Page MenuHomePhabricator

Add shadow to nav menu drawer
Closed, ResolvedPublic2 Estimated Story Points

Description

Description

To be consistent with other drawers, and to help clarify the spatial relationship between the drawer and the page behind, the nav menu drawer should have a shadow.

Design

box-shadow: 1px 0px 8px 0px rgba(0,0,0,0.35);

currentwith shadow

Acceptance criteria

  • The main menu has the box shadow specified by the design section above
  • Immortalise the color for the box shadow in a variable and make sure that variable is used for the drawers at the bottom of the page, the main menu and the new notifications T231205#5437984. The notification drawer box shadow color is the same as the main menu.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 26 2019, 2:53 PM
ovasileva triaged this task as Medium priority.Aug 26 2019, 3:00 PM

@alexhollender, should we update the notification drawer shadow?

@alexhollender, should we update the notification drawer shadow?

I'm not opposed to that, though if we proceed with the changes proposed in T226125 I don't think it will matter.

Jdlrobson updated the task description. (Show Details)Oct 21 2019, 5:25 PM
Jdlrobson set the point value for this task to 2.
N3rsti claimed this task.Dec 21 2019, 5:01 PM

Change 560192 had a related patch set uploaded (by N3rsti; owner: N3rsti):
[mediawiki/extensions/MobileFrontend@master] mobile.variables.less: added drawerShadow variable Drawer.less added shadow to navigation-drawer class

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

Change 560192 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] mobile.variables.less: added drawerShadow variable Drawer.less added shadow to navigation-drawer class

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

N3rsti closed this task as Resolved.Dec 23 2019, 12:35 PM
Jdlrobson reopened this task as Open.Dec 23 2019, 6:17 PM
Jdlrobson added a subscriber: Jdlrobson.

Seeing a regression from this change. I will revert.

@Jdlrobson My GCI task was already approved, but I see that there is a problem with it. Can I fix it?

@Jdlrobson My GCI task was already approved, but I see that there is a problem with it. Can I fix it?

Sure - we'll be happy to see that fixed!

I tried to reconstruct this bug on devtools and I see this shadow on the left side (which I will try to fix), but I don't see the shadow on the burger menu. On which website have you seen this bug?

I tried to reconstruct this bug on devtools and I see this shadow on the left side (which I will try to fix), but I don't see the shadow on the burger menu. On which website have you seen this bug?

Hi, this change hasn't been deployed to Wikimedia wikis yet. So you can not reproduce this regression on mediawiki.org. It seems the screenshot is of the main page of English Wikipedia beta cluster.

I tried to reconstruct this bug on devtools and I see this shadow on the left side (which I will try to fix), but I don't see the shadow on the burger menu. On which website have you seen this bug?

Hi, this change hasn't been deployed to Wikimedia wikis yet. So you can not reproduce this regression on mediawiki.org. It seems the screenshot is of the main page of English Wikipedia beta cluster.

It is probably also worth mentioning that you will no longer be able to replicate this bug on the Beta cluster, as the change was reverted. You will have to debug this on your local test instance.

It is probably also worth mentioning that you will no longer be able to replicate this bug on the Beta cluster, as the change was reverted. You will have to debug this on your local test instance.

Right. I didn't realize it was already reverted.

Change 560442 had a related patch set uploaded (by N3rsti; owner: N3rsti):
[mediawiki/extensions/MobileFrontend@master] mobile.variables.less: added drawerShadow variable Drawer.less fixed shadow in navigation-drawer class

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

N3rsti added a comment.EditedDec 23 2019, 9:43 PM

It should be working now, it it's not, please let me know and I will try to fix it.

On this page, where the bug was, I didn't add any shadow, because there was already one (it's a little bit different, but it's not a big difference in my opinion), but if you want to have the exact same shadow like on other pages, I can fix it.

It should be working now, it it's not, please let me know and I will try to fix it.

On this page, where the bug was, I didn't add any shadow, because there was already one (it's a little bit different, but it's not a big difference in my opinion), but if you want to have the exact same shadow like on other pages, I can fix it.

Of course! I'll be more responsive this time!

(it's a little bit different, but it's not a big difference in my opinion), but if you want to have the exact same shadow like on other page

The goal here is consistency so we want to make sure every box shadow is the same. So I see the following todos..

  • The color for all the above should be defined in a control variable in minerva.less/minerva.variables.less
  • .drawer currently has box-shadow: 0 -1px 8px 0 rgba( 0, 0, 0, 0.35 ); (remains unchanged) and the color uses the new variable.
  • #mw-mf-page-left currently has no box-shadow. Needs box-shadow: 1px 0px 8px 0px rgba(0,0,0,0.35);
  • .notifications-overlay.navigation-drawer currently has box-shadow -5px 0 0 0 rgba(0,0,0,0.3). Alpha color should be changed to 0.35

Change 560519 had a related patch set uploaded (by N3rsti; owner: N3rsti):
[mediawiki/skins/MinervaNeue@master] minerva.variables.less: added gray color variable MainMenu.less: added shadow to navigation NotificationOverlay.less: changed alpha color of shadow drawers.less: replaced rgba color with the same color variable

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

It should be fixed now. I'm not sure if I created variable in the right place, but it's easy to fix in necessary.

Change 560519 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Fix shadow on notification overlay and navigation-drawer

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

Leaving this open so we can run this by our designer when he's back in January.

On beta it looks like the correct shadow is being applied.

screenshot of betashadow css

There is a 2px border-radius being applied via .toggle-list__list that should not be there (or should be overridden).

Also I'm curious where else we use .toggle-list__list? It has a box-shadow of its own that is quite nice, and a little less intense than our current default.

N3rsti added a comment.Jan 2 2020, 6:22 PM

@alexhollender I haven't changed border-radius, because from the task I had to change color only. I did everything from this list:

The goal here is consistency so we want to make sure every box shadow is the same. So I see the following todos..

  • The color for all the above should be defined in a control variable in minerva.less/minerva.variables.less
  • .drawer currently has box-shadow: 0 -1px 8px 0 rgba( 0, 0, 0, 0.35 ); (remains unchanged) and the color uses the new variable.
  • #mw-mf-page-left currently has no box-shadow. Needs box-shadow: 1px 0px 8px 0px rgba(0,0,0,0.35);
  • .notifications-overlay.navigation-drawer currently has box-shadow -5px 0 0 0 rgba(0,0,0,0.3). Alpha color should be changed to 0.35

I think .toggle-list__list class is used in navigation bar on the wikipedia beta main page only (I haven't seen it anywhere else) and it has shadow from #mw-mf-page-left id so it overrides previous shadow. I can fix it with #mw-mf-page-left.navigation-drawer so it will be used only on pages like this . I'm not sure, but I think this file change was reverted so you can't see the shadow now (.primary-navigation-enabled is body class when navigation is opened so it only displayed nav shadow if opened). So I think things to do are:

  • Remove border-radius
  • Decide which shadow should be used in .toggle-list__list and if we should use previous one, I can fix it like I said before.

I've already completed this task because it was only to change colors and add variable, but I will be happy to help you with this problem.
Let me know if I can fix and which shadow should be used.

@N3rsti ok for now I think all that needs to be done is removing the border-radius.

@Jdrewniak do you know what the deal is with .toggle-list__list and why it has a box-shadow?

N3rsti added a comment.Jan 6 2020, 7:16 PM

@Jdlrobson Why was this change reverted? I know you said that "this CSS block is dead", but now it doesn't show shadow in websites like this . There were no other shadow styles for this drawer.

Change 562336 had a related patch set uploaded (by N3rsti; owner: N3rsti):
[mediawiki/skins/MinervaNeue@master] Remove border-radius

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

N3rsti added a comment.Jan 6 2020, 7:33 PM

I removed border-radius from .toggle-list__list. I haven't found any other border-radius styles that could override this so removing it from ToggleList.less should be fine, but I'm not sure why previous box-shadow change was reverted, like I said before.

@Jdlrobson Why was this change reverted? I know you said that "this CSS block is dead", but now it doesn't show shadow in websites like this . There were no other shadow styles for this drawer.

@Jdlrobson Why was this change reverted? I know you said that "this CSS block is dead", but now it doesn't show shadow in websites like this . There were no other shadow styles for this drawer.

Good question. I'm guessing there was a potential problem with the coordination of the two patches (the one you link and the one referenced in the commit message ( If4831fc700c7df3a2a389b5f95b6fbaea4b7d954 ) . https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page runs the latest code and I see the box-shadow there but not the primary-navigation-enabled class. It's most likely this is a regression which will resolve itself in the next deploy.

There is a 2px border-radius being applied via .toggle-list__list that should not be there (or should be overridden).

The border-radius is being used on the overflow menu and now the main menu and this menu share code. I assume we want to keep the border radius here?

The border-radius is being used on the overflow menu and now the main menu and this menu share code. I assume we want to keep the border radius here?

Yes, just remove it from the main menu

N3rsti added a comment.Jan 6 2020, 8:19 PM
This comment was removed by N3rsti.
N3rsti added a comment.EditedJan 6 2020, 8:41 PM

https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page runs the latest code and I see the box-shadow there but not the primary-navigation-enabled class. It's most likely this is a regression which will resolve itself in the next deploy.

Shouldn't the shadow be used on drawers on, for example this site? I know it's mediawiki website, not wikipedia beta, but GCI Task name and description was about MobileFrontend extension so I thought it should be used in mediawiki websites using MobileFrontend extension as well, but on mediawiki website i think it should be done in mobilefrontend code, not minerva skin (but i'm not sure)

https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page runs the latest code and I see the box-shadow there but not the primary-navigation-enabled class. It's most likely this is a regression which will resolve itself in the next deploy.

Shouldn't the shadow be used on drawers on, for example this site? I know it's mediawiki website, not wikipedia beta, but GCI Task name and description was about MobileFrontend extension so I thought it should be used in mediawiki websites using MobileFrontend extension as well.

it should yes and this is likely because only 1 of the 2 patches got deployed, but I don't think it warrants further investigation as it will be fixed in the next round of deployments (code on wikipedia beta will be on mediawiki.org later tomorrow).

N3rsti added a comment.Jan 6 2020, 8:53 PM

Ok, but I don't think it will work without this change which was reverted. Should I add it again (if it won't be a problem now)?

Change 562336 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove border-radius

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

Ok, but I don't think it will work without this change which was reverted.

are you seeing a problem with https://en.m.wikipedia.beta.wmflabs.org/ ? The box shadow looks fine to me there?

Should I add it again (if it won't be a problem now)?

No additional work is needed provided https://en.m.wikipedia.beta.wmflabs.org/ is working as expected. We deploy the code running on https://en.m.wikipedia.beta.wmflabs.org/ every week to all our wikis including mediawiki.org.

@alexhollender @N3rsti has fixed the border-radius issue so you should be able to test this on https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page

N3rsti added a comment.EditedJan 6 2020, 9:55 PM

It's working on wikipedia beta because it use #some_input_id:checked ~ #mw-mf-page-left (i don't remember id), but from what I've seen mediawiki is using anchor in this id so it can't be checked. Now I'm on mobile phone so I can't check dev tools, but I'm pretty sure there was anchor, not input on mediawiki

Edit: You're right. Sorry, I didn't know about HTML changes

Change 560442 abandoned by Jdlrobson:
mobile.variables.less: added drawerShadow variable Drawer.less fixed shadow in navigation-drawer class

Reason:
Taken care of in https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/ /560519/

Thank you N3rsti!

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

alexhollender removed N3rsti as the assignee of this task.Jan 8 2020, 6:11 PM
alexhollender added a subscriber: N3rsti.

Looks good on beta:

thanks @N3rsti !

ovasileva closed this task as Resolved.Jan 9 2020, 10:25 AM
ovasileva claimed this task.