Page MenuHomePhabricator

Banner/CentralNotice should be child of `header` and not above it
Open, NormalPublic

Description

CentralNotice should be part of header element (or, if you prefer so, some other landmark like main) in order to be part of an ARIA landmark.
Currently it's placed above header and outside of any landmark.

Developer notes

This essentially means moving banner-container.
We can move it either above or below the header
but given the header has a gray background mobile banners would be expected to set an appropriate background (not white like in Vector)

Please advise whether above or below the header is required.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 24 2018, 11:25 PM
Jdlrobson updated the task description. (Show Details)

@Volker_E can you provide a screenshot of the Banner/CentralNotice you refer to?

@alexhollender For example this one:

I don't know if there are any downsides (and I guess we'd need to sync with fundraising) but having banners under the Wikipedia header would mean no more random accidental banner clicks which would make me happy!
cc @Nirzar

This probably isn't an issue for fundraising (we position our banners differently anyway) but @Jseddon might be interested in the impact on community banners.

alexhollender added a comment.EditedSep 27 2018, 5:14 PM

So Nirzar created this at some point last year (I believe), which shows a direction for putting these banners under the main Wikipedia header:

structureexamplesfull spec

I think it's a good direction for these banners. From a content hierarchy standpoint it makes more sense to put the banner below the header. Is there any chance we could make a deeper investment into the template that controls the styling of the banner itself here? Containing the size of these, and making a larger and more reliable tap area for closing them would be a huge improvement to the overall reading experience.

having banners under the Wikipedia header would mean no more random accidental banner clicks which would make me happy!

@Jdlrobson why would there be fewer accidental clicks in that case?

@Jdlrobson why would there be fewer accidental clicks in that case?

Right now the banner loads late. So if you are going to wikipedia to start a search or go to nearby/watchlist, it's very possible to go to click the search/hamburger and then suddenly find you are clicking the banner. This happens so much to me and drives me insane.

@Jdlrobson why would there be fewer accidental clicks in that case?

Right now the banner loads late. So if you are going to wikipedia to start a search or go to nearby/watchlist, it's very possible to go to click the search/hamburger and then suddenly find you are clicking the banner. This happens so much to me and drives me insane.

+1

Change 463344 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Banner now nested inside header

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

Change 463345 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Banners have sensible default styles to fit in with Minerva's design

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

Jdlrobson added a comment.EditedSep 27 2018, 7:07 PM

Above two patches make it possible for us to run banners like this:
http://reading-web-staging.wmflabs.org/w/index.php?title=Main_Page&mobileaction=toggle_view_mobile

I'm using wgSiteNotice to save setting up CentralBanner but for prosperity the banner HTML looks like this:

$wgMinervaEnableSiteNotice = true;
$msg = 'Imagine a world where our banners look great on mobile.<br>'
.'Imagine.<br>'. '\'\'\'[https://donate.wikimedia.org/w/index.php?title=Special:LandingPage&country=US&uselang=en&utm_medium=sidebar&utm_source=donate&utm_campaign=C13_en.wikipedia.org <span style="color:#fff">Just imagine.</span>]\'\'\'';
$img = 'https://upload.wikimedia.org/wikipedia/commons/thumb/6/6e/Schrecklicherpfeilgiftfrosch-01.jpg/80px-Schrecklicherpfeilgiftfrosch-01.jpg';
$wgSiteNotice = "<div class='mobile-blue-banner' style='text-align: left; background: #315ad3; color: white; padding: 8px; display: flex;'>"
        . "<div>[[File:Schrecklicherpfeilgiftfrosch-01.jpg|frameless|thumb|100px]]</div>"
        . "<div style='padding: 0 8px;'>$msg</div>"
        . "</div>";

Change 463345 abandoned by Jdlrobson:
Banners have sensible default styles to fit in with Minerva's design

Reason:
Not necessary

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

Any problems with making this happen @alexhollender (moving the position?)
If so, @Volker_E feel free to +2 https://gerrit.wikimedia.org/r/463344

@Jdlrobson can you explain a bit about how banners work and what your patch is doing? Or point me towards something to read? I have almost zero context so don't feel comfortable moving forward so quickly.

Also, I don't see a close button on the banner you linked to.

Jseddon added a comment.EditedSep 28 2018, 11:37 AM

I'm not intrinsically opposed to moving banners beneath the search bar but I ask this change be held off until January.

We are entering a code freeze on CentralNotice due to the Big English fundraiser and only high priority changes should be made.

We are also have live campaigns running and I would like to do a thorough check to make sure there wouldn't be any unexpected behaviour with any currently running community or fundraising banners

Jseddon triaged this task as Lowest priority.Sep 28 2018, 11:43 AM

Is accessibility classed as a high priority change?

@Jdlrobson and @alexhollender I apreciate the input and I'm happy to talk about this after Q2. I personally have no objections to trying this. However there needs to be a great deal of banner testing around this. It might not have a material effect on banners or it might end up requiring Advancement to make major design changes.

A change like this seems pretty significant at this time. We're now in code freeze for Central Notice. Around this time of the year, anything that affects banners needs to be discussed with Advancement and fr-tech. Significant changes are typically held off until January.

Adding more as fyi: @EBjune @mepps @Ejegg @MeganHernandez_WMF

Again: this change looks fine from a fundraising perspective and should have no effect on our banners. We use javascript within the banners to move them into the desired location (in fact our mobile large banner is already placed in the proposed position).

I'm not opposed to this but I want to be able to A/B test this change within programmatic banners prior to committing to it. (which I should be easily be able to do within a campaign)

The biggest impact will be on programmatic banners which use the default location. I just want to make an informed decision before committing to it.

At the same time I can fully test Nirzar's proposed mobile designs.

DStrine added a subscriber: Seddon.Sep 28 2018, 5:56 PM

+1 to @Seddon testing. I believe this will be on the beta cluster if it gets +2? Fr-tech would also like to check @Seddon 's test just to be sure everything is looking good on our end.

Change 463344 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Banner now nested inside main article at top

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

@alexhollender letting you know this got merged.
Is there anyway we can test the new banner from @Seddon on the beta cluster?

no action yet, just adding a note from a discussion with Nirzar so I don't loose it — the close circle is supposed to be 44px total, so currently too big

alexhollender raised the priority of this task from Low to Normal.Feb 27 2019, 6:08 PM

Just circling back to review this. Close button is now 44px.

One issue. When the interface is not width constrained (window >1000px) it seems there is :

#content {
    border-top: 1px solid transparent;
    padding-bottom: 32px;
}

For now I've put a 20px padding.

The banner now also handles narrow resolutions much better with tweaks that should handle the majority of ultra narrow resolutions.

@Jseddon Small comment, without having access to Nirzar's original designs. The background on the close icon seems to me to give that element too much visual focus. The message should be in the focus and then a clearly indicated way to abort the message.

@Jseddon apologies for the delay in reviewing/responding here. The links in T205360#5149266 no longer seem to work, would it be possible to post updated links?

Having this bug open with the title "Banner/CentralNotice should be child of header and not above it" (which is implemented) is very confusing. Please can you revise the description and title if we are repurposing this (or maybe create a new task to track remaining work)

@Volker_E not sure what we're doing with this task. Are there visual implications here? If not can we remove it from the design board?