@ovasileva this has now been merged, but it is featured flagged and is turned off by default so we'll have to swat deploy to turn it on. The deploy should be pretty easy we only need to set the following config flags:

All of your requested changes are now up on http://readers-web-staging.wmflabs.org.

Should the button say "Enable advanced mode"? I think it could be good to reinforce what you're doing by clicking the button.

Padding is restored

@alexhollender As mentioned in slack this is now up for design review at readers-web-staging.wmflabs.org, but it's a bit of a pain to test multiple times because it should only show once per browser. A way to get around this is to use a new incognito mode tab in chrome (looks like ios safari might have their own version of that at https://support.apple.com/en-us/HT203036) every time you want to see the drawer. If that’s too much of a pain or doesn’t work, I can add more code to make it easier. To see the drawer now:

pinging @Jdlrobson since I only code reviewed although I'm having trouble reproducing what alex is seeing at least on my laptop

In T225747#5396682, @alexhollender wrote:

@nray @ovasileva not sure what the sequencing will end up being, but wanted to note that whenever we update the setting name here we should also update it in the message in T226068. Maybe it makes sense to add as an acceptance criteria to this task?

@alexhollender Thanks for clarifying!

@alexhollender sorry for the repeated pings, another question came up

@alexhollender I have three questions for you that are specific to this ticket:

people see the drawer once per device/browser combo, rather than once total
people don't see the drawer at all if another user on the same device/browser combo has already seen it

I've moved this into code review. In my latest patch I implemented the feature using local storage (it was mostly code removal) and pretty easy so I figured it was worth doing even if we end up preferring to use the database.

As this task has lingered, I want to provide an update on what's happening:

@jcrespo (cc: @phuedx) To implement this feature and given that there is a requirement that the drawer can only be shown once regardless of the device (I will be questioning this requirement in a later post), I think we need to make use of the user_properties table and add a record to that table when the drawer is shown. That record would then serve as a flag to not show the drawer again. However, I'm wondering if the number of records this will add to the database is an acceptable number. It would basically work like this:

@alexhollender I was thinking for simplicity it would work like this: After the Outreach feature is released and turned on, if the user turns off AMC from their settings page, they would not be eligible to to see the Outreach modal ever again.

@alexhollender @ovasileva I'm thinking we need to make it so that if the user has AMC enabled and then turns it off in their settings, they will also not be eligible to see the outreach modal (even if they haven't seen the outreach modal before). Otherwise, when this feature lands, if people who currently have AMC enabled try to disable it in their settings, they would see the modal appear as soon as they turn off AMC and that would probably be pretty irritating.

@alexhollender the task description shows a tablet icon on the drawer; where can I find this icon?

@Edtadros sorry, I needed to update the QA instructions. You should be able to see the print button if you follow the instructions now

@Edtadros this should be fixed now if you check the page again. Thanks for noticing this and bringing it up!

In T226025#5318006, @Jdlrobson wrote:

@nray could we just use vh units here for max-height instead?

There are multiple things to the Drawers that contribute to the performance problems. Here is the profile of it on the Obama page when the first reference is clicked using 6x throttle to simulate a mobile device:

In T225768#5335620, @Jdlrobson wrote:

I see this button is an input element. Is that why? Do we know why this is happening?

In T206354#5317831, @Jdlrobson wrote:

Do we expect this to be a CSS only change or require some JS changes? Needs a summary from a developer to make this something we can estimate.
Seems like Nick has a good grasp of this so can I ask that you summarise T206354#5209466 in the description so it has acceptance criteria and/or developer notes?

The dependency order on these patches is fairly confusing so here are the patches that still need review starting with the order they should be merged in:

Moving this back to To Do as I'm interested to hear Gilles's advice to my initial analysis above

Functionally, doesn't disabling the lazy loading logic in the case of the section toggling mean that lazy-loaded images that might end up inside the viewport as a result of section expansion (top images inside the previously collapsed section) won't load until the user scrolls?

@Gilles Okay, today I carved out time to do some analysis on this. Here were my findings and my interpretation of the findings. Please correct me where I'm wrong!: