Fri, Sep 8
@ovasileva i thought we are stalling this as this is scope creep to print work
@Nirzar See https://phabricator.wikimedia.org/T172902#3589643. The patch is just exploratory work to see how we might do this - a proof of concept. It has no chance of getting merged or going any further without us committing time.
Any ideas for how to do this @Legoktm? We've been brainstorming ways over in the phabricator ticket T172902 for the print mode and it would be good to get some input on how best to do this (the patch you just reviewed was a strawman proposal to show one way it might be done). I like the idea of something more generic but I'm not sure what that would look like.
issues have been addressed! @pmiazga is on it!
@bmansurov on irc said he can have a go at signing this off
https://gerrit.wikimedia.org/r/374908 has a +1.
I am going on vacation later today.
Interesting. I didn't realise we didn't allow for sessions for anonymous users. I'm curious how this works with Varnish now.
Why has this and T174957 been moved to do? The work has been done (so this probably should be code review), but I'm reluctant to review this until https://phabricator.wikimedia.org/T174957#3585819) has an answer. @MBinder_WMF how would we capture this on a kanban board?
> We are building a new service that will probably be serving all the HTML for all our clients for the foreseeable future…we don't want to have to make breaking changes in 2 years to support something we never took a few moments to discus on Phabricator early on in the process.
Thu, Sep 7
can we generate it and show the size and then let the person download? that was meant to be the flow.
I still say we keep it in tracking or backlog.
What is the goal at doing this?
See also https://phabricator.wikimedia.org/T174955#3588362 from @phuedx
Phabricator is not a great place for tracking things that cannot be fixed or we don't know how to fix. It makes it harder to actually find things that should be worked on.
Change should roll out in 1.30.0-wmf.18 which I think will be live on 13th.
We can't do the SWAT deploy until T173018 is done (otherwise we'll be creating a broken experience) so I've merged these to get them done together.
^ the above patch implements one of the proposals. Would be good to talk about it vs other proposals in next grooming session.
I assume this is a precursor to T169732 (ie. the same but only for beta cluster)? If so this doesn't seem stalled. There should be no reason not to enable them on beta cluster... although I'm not sure how useful this is. There are very few articles on the beta cluster which render nicely and we're testing on live data on https://reading-web-staging.wmflabs.org.
Pulling into sprint since this has been merged and is being SWATTed today (presumably due to the noise in logstash) and it's important we verify the fix.
Could we update which configuration variable that would be? It's very vague right now to know what this is referring to.
What exactly needs deploying here? ElectronPdf is already deployed to all wikis.
Is there any deploy necessary here? I thought everything is deployed already - it's just a case of the service has not been fully built yet?
Do you mean "new Vector print styles" e.g. the ones we've been working on in T154965?
I've written a proof of concept patch, but while doing this I uncovered the fact that this is blocked by T167713.
I have updated the description with a proposed solution.
I think this is ready to discuss/estimate at our convenience.
This has been lingering in sign off for some time and I wonder if there is a better way to capture the remaining problems, in a "verify A/B test card" that lives in the blocked column and has a clearer outline on what the remaining problem is here. The A/B test has been launched so it's misleading to leave this open (cc @MBinder_WMF ).
Looks like Sam might be signing this off? Please re-assign if that's not the case.
Stephen would you mind signing this off since I'll be away on vacation (note the "verify fix" steps for now and later in the description)?
Yes but you please verify this on the beta cluster. Any issues we'll want to catch and fix before the train rolls out on Tuesday:
Per acceptance criteria there is still some refactoring for review:
That was an easy fix..
@Jdlrobson, hi Jon. Sorry for the noob question but why do we need display: table-cell on .tocnumber, .toctext
It looks like this is used to control how the table of contents displays on long lines. I don't know how attached we are to that. We could easily disable this in mobile.
And yes this is related and another possible solution. The problem being that if the table does not express a width, it will consider the table of contents a table and assume it cannot break the content flow.
Why are we overcomplicating this? Just remove them like we do everywhere else. We can change this decision later if the Navbox template works on desktop, but that's not happening any time soon. Let's not over-engineer this.
@Jdlrobson is development on the new end point still stalled while OCG is worked out? Or is it moving forward?
I'm working on it part time, but you should think of it as stalled. That said, I strongly believe the new endpoint is superior to the old and that's what I'd like to prove.
const UserLoginPage = require( '../../../../../tests/selenium/userlogin.page' ); UserLoginPage.login( username, password );
great that was what I was looking for.