Page MenuHomePhabricator

Add project wordmark to print styles
Closed, ResolvedPublic3 Story Points

Description

In order to raise awareness of our projects, we need to include the project wordmark to the the print styles of articles

Design

Zeplin Spec
https://zpl.io/26T0fw

Where does the logo come from?
We recently added project wordmarks to mobilefrontend headers, We would like the same project wordmark to appear here.

Developer notes

  • This is going to be a little tricky. We need to include an entry in a configurable variable ($wgMinervaCustomLogos) inside a css file.

The ResourceLoaderSkinModule.php does exactly this. Options are subclassing ResourceLoaderFileModule or registering a LESS variable inside onResourceLoaderGetLessVars (see @matmarex's super useful comments > T169826#3415344)

Test plan

Related Objects

StatusAssignedTask
OpenJKatzWMF
OpenNone
DuplicateNone
OpenJKatzWMF
OpenNone
OpenNone
Resolvedpmiazga
InvalidNone
OpenNone
Resolvedovasileva
InvalidNone
Resolvedphuedx
OpenNone
InvalidNone
InvalidNone
OpenNone
DuplicateNone
ResolvedNirzar
ResolvedNirzar
OpenNone
Declinedmobrovac
StalledJdlrobson
DeclinedNone
StalledNirzar
ResolvedNirzar
OpenNirzar
ResolvedNirzar
Resolvedovasileva

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

thanks @matmarex that's super helpful advice.

If someone wants to work on this feel free to base off of https://gerrit.wikimedia.org/r/366173

Change 366315 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/skins/Vector@master] Add print logo

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

The ResourceLoaderSkinModule.php does exactly this, but it's not done in a very generic way. We'll either have to extend this or adjust the implementation in core.

A generic way to generate CSS depending on config options is to subclass ResourceLoaderFileModule and override the getLessVars() method. You can then write your styles as Less like you normally would, and use the extra magically generated variables in it. See ResourceLoaderEditToolbarModule for an example.

Indeed. Per T140804, do not introduce new usage of this hook.

Nirzar added a comment.EditedJul 20 2017, 1:49 AM

@bmansurov sincere apologies for change in position of wordmark mid development. a use-case was unearthed while developing T169823 and a follow-up task has been filed for it here T171114 which impacts this task in terms of the position of wordmark.

I try to avoid such things but I think I dropped the ball on this one.

Zeplin has been updated.

change requested: moving wordmark from right hand side to left hand side above the title of the article. similar to our mobile print styles

@Nirzar np. I also noticed that the mocks don't take long multi-lined titles into account. Although that's not a problem with the new watermark location change, you may want to add new mocks to show how indicators should be positioned when the title spans multiple lines. Here is an example title.

matmarex removed a subscriber: matmarex.Jul 22 2017, 7:49 PM

The icons on right will be bottom aligned with the border bottom of H1

wrapping text around it would be wonderful

@Nirzar are we ok not showing the wordmark for PDFs printed via IE8? If we use SVG's it will make the configuration simple, but IE8 won't be able to handle it. Otherwise, we can use PNG's but have a little complex configuration file.

are we ok not showing the wordmark for PDFs printed via IE8?

yeah.. we can drop wordmark support for IE8

Just to make sure, for IE8 then, we'll be displaying text?

No, broken image placeholder, something like T166684.

@bmansurov T

That's not what I'm seeing. Your current implementation shows no image for IE8. Not a broken image placeholder.

That's right. I just realized that.

Change 366315 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Add print logo

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

bmansurov removed bmansurov as the assignee of this task.Aug 2 2017, 2:05 AM
bmansurov added a subscriber: matmarex.
matmarex removed a subscriber: matmarex.Aug 2 2017, 5:56 PM

when clicking print the wordmark sometimes doesn't appear on the printed page. Baha also said that the image width/height are showing up as 0. Let's debug why.

bmansurov claimed this task.Aug 3 2017, 7:34 PM
bmansurov moved this task from Needs More Work to Doing on the Readers-Web-Kanbanana-Board-Old board.
bmansurov added a subscriber: matmarex.

when clicking print the wordmark sometimes doesn't appear on the printed page.

@Nirzar I remember you telling that this was the case. Can you share your browser info and the steps to reproduce this? Is the logo not visible in preview mode or in the final PDF?

OK, turns out there were two configuration options in for the same configuration variable in different places. I've removed the one in LocalSettings.php and kept the one in 10-Vector.php.

bmansurov removed bmansurov as the assignee of this task.Aug 3 2017, 7:59 PM
Jdlrobson claimed this task.Aug 3 2017, 8:15 PM

. I've removed the one in LocalSettings.php and kept the one in 10-Vector.php.

Did you mean the opposite? The latter is written every time vagrant updates. I'm seeing the rule in:
/srv/mediawiki-vagrant/LocalSettings.php

I'm still not seeing the logo on several pages as reported by @Nirzar and @ovasileva so I will take some time to investigate that.

I created 10-Vector.php inside settings.d and I don't think vagrant touches those files. The config is in that file. Not sure why you see it in LocalSettings.php.

jdlrobson@reading-web-staging-3:~$ sudo su mwvagrant
mwvagrant@reading-web-staging-3:~$ cd /srv/mediawiki-vagrant/
mwvagrant@reading-web-staging-3:/srv/mediawiki-vagrant$ ag MinervaCustomLogos
LocalSettings.php
165:$wgMinervaCustomLogos = [

I see. Look for $wgVectorPrintLogo.

Got it. But yes those should be /srv/mediawiki-vagrant/LocalSettings.php

Krinkle added a comment.EditedAug 3 2017, 8:40 PM

/vagrant/settings.d/*.php is for your own custom settings files. (As indicated README, and 00-debug.php-example). The subdirectory /vagrant/settings.d/puppet-managed/ is indeed managed by puppet. There, any modifications or extra files will be overwritten.

The same also applies per-wiki. /vagrant/settings.d/wikis/$mywiki/*.php and /vagrant/settings.d/wikis/$mywiki/settings.d/puppet-managed/*.php are managed by puppet. And any files matching /vagrant/settings.d/wikis/$mywiki/settings.d/*.php may be your own.

Personally, I limit my custom settings to just this one single file /vagrant/settings.d/10-mystuff.php. I leave everything else alone (including no changes to root LocalSettings.php, because it already contains various defaults and is thus harder to maintain).

Thanks for clarifying this @Krinkle is there a wiki page I can add to team's bookmarks which explains this?

At the top of /srv/mediawiki-vagrant/LocalSettings.php I see:

* To customize your MediaWiki instance, you may change the content of this
* file. See settings.d/README for an alternate way of managing small snippets
* of configuration data, such as extension invocations.

Sound like this is misleading?

Jdlrobson removed Jdlrobson as the assignee of this task.Aug 3 2017, 10:16 PM

Okay.. I've worked out the problem here. It's very similar to lazy loading images.
The problem here, is that the background image will only get loaded when the print function is async.

As I mentioned in the spike T171162 the print function is synchronous.

This is what I see happening in Chrome:

The issue is because:

  • Clicking print is synchronous action. In background image is loaded, but when loaded the preview does not get refreshed.

Possible solutions:

  1. Apply the css on all page views (don't restrict to print media query), but use display: none to hide in non-print mode.
  2. Add a JS event handler that hooks into the onbeforeprint action to load the image (untested - might not work)
  3. Load the image into the browser cache via a call to new Image( that occurs inside a setTimeout.
matmarex updated the task description. (Show Details)Aug 4 2017, 11:24 AM
matmarex removed a subscriber: matmarex.
bmansurov claimed this task.Aug 4 2017, 2:11 PM
bmansurov moved this task from Needs More Work to Doing on the Readers-Web-Kanbanana-Board-Old board.

@bmansurov how are you planning to solve this? Would be good to talk through the options!

@Jdlrobson thanks for looking into the issue.

Apply the css on all page views (don't restrict to print media query), but use display: none to hide in non-print mode.

The browser would still not load the image because it's hidden by default, would it?

The other two options you mention require JavaScript, so I think we should look for another alternative. Maybe use data-URI's.

Change 370221 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/skins/Vector@master] Embed print wordmark

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

I've submitted a patch to embed the logo, but that does still not work on Firefox for some reason. It only works when generating a print preview, but the wordmark is still invisible when a PDF is printed directly (without generating a preview).

Change 370221 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Embed print wordmark and pre-render it

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

Jdlrobson reassigned this task from bmansurov to ABorbaWMF.Aug 7 2017, 5:00 PM
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson updated the task description. (Show Details)

@pmiazga, sorry for the delay here. I am seeing the same behavior that @Jdlrobson reported above when I try it on chrome.

"This is what I see happening in Chrome:

Visit http://reading-web-staging.wmflabs.org/wiki/William_Cooley and click native browser function
View preview (Chrome) - wordmark is not visible
Close dialog
Click print again
Wordmark is visible"

So far it looks good on Safari and Edge.

Please can you look again @ABorbaWMF looks like some old code was running on staging.

Yep, looks good now on Chrome too

The space between the wordmark and the H1 title is not correct

https://zpl.io/VQMYAAV

spec here ^

The spec says 18.1px. Surely, it's not right. What should the value be, @Nirzar?

Oh it says 18.1 because of line height for H1, zeplin seems to take wrong viewbox for the H1 I will report a bug. the correct value is 20px.

Sorry about that

bmansurov moved this task from Needs More Work to Doing on the Readers-Web-Kanbanana-Board-Old board.
bmansurov added a subscriber: ABorbaWMF.

Change 373141 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/skins/Vector@master] Change print wordmark margin bottom to 20px from 10px

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

Change 373141 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Change print wordmark margin bottom to 20px from 10px

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

Jdlrobson reassigned this task from bmansurov to Nirzar.Aug 23 2017, 2:11 PM

back to you for sign off. It's been updated on staging.

Nirzar closed this task as Resolved.Aug 23 2017, 4:50 PM

Macro votecat: looks  good