cc @Nirzar and @phuedx - something we want to consider or are we tied to 300 by 300? Switching to 320 does seem like it would be a sensible thing to do...

we should consider this. looks like the benefit will be higher than the change required. I will render some mocks and see how it might look. plus i like the thought behind retina.

So it won't make a big difference in design. for what it's worth, it will make page previews wider than taller, which is a good thing, since desktop screens have more width than height.

upside
I wanted to make sure I understand the upside of this. since 320 is more common size, hitting the cache for new images is more likely but what happens 6 months down the line? is the performance improvement gives diminishing returns?

downside
We're are at last phases of page previews. doing a layout change scares me at this point. regressions, ui bugs? we have to evaluate the risk of making a change vs the upside. I will leave that to engineering to decide.

and I will defer to product for the final decision cc @ovasileva

Bottomline design recommendation: Let's do it, if it's low effort and we are sure there won't be bugs

I wanted to make sure I understand the upside of this. since 320 is more common size, hitting the cache for new images is more likely but what happens 6 months down the line? is the performance improvement gives diminishing returns?

Yes. This means less stress on the servers that serve Wikipedia (as witnessed by the spike we caused in T173422) and quicker responses for images for our users. There are no diminishing returns. Provided we use commonly used thumbnails they will always be quicker than if we don't.

From my perspective doing this seems like a no brainer. Fear of bugs shouldn't be used as an excuse for not doing something which is sensible. Not doing this, we're just delaying the problems associated till a later date.

Yes. This means less stress on the servers that serve Wikipedia (as witnessed by the spike we caused in T173422) and quicker responses for images for our users. There are no diminishing returns. Provided we use commonly used thumbnails they will always be quicker than if we don't.

That's good to know.

From my perspective doing this seems like a no brainer. Fear of bugs shouldn't be used as an excuse for not doing something which is sensible. Not doing this, we're just delaying the problems associated till a later date.

Given all this information, I don't think anyone would oppose doing this.
I wouldn't quite put it as "fear of bugs" though. It's more of a risk assessment

In T173434#3545169, @Jdlrobson wrote:

I wanted to make sure I understand the upside of this. since 320 is more common size, hitting the cache for new images is more likely but what happens 6 months down the line? is the performance improvement gives diminishing returns?

Yes. This means less stress on the servers that serve Wikipedia (as witnessed by the spike we caused in T173422) and quicker responses for images for our users. There are no diminishing returns. Provided we use commonly used thumbnails they will always be quicker than if we don't.

Just a quick follow up that, per T173422#3537935, that spike wasn't caused by rolling out Page Previews to 100+ wikis but it may have been exacerbated by it. However, I don't think that this detracts from the point that maximising cache hit rate is A Good Thing™.

Sounds good to me. Tagging with the sprintboard and moving to needs analysis (unless triaged but future is more appropriate)

Would be great to make a decision on this.

In T173434#3586006, @Jdlrobson wrote:

Would be great to make a decision on this.

I think the decision was made in T173434#3548129.

@ovasileva: Is Upcoming appropriate?

> @ovasileva: Is Upcoming appropriate?

That works, although I don't expect us to get to this in the next sprint or two.

If the task intent has been resolved, can we mark this good first task?

@Niedzielski: Yes! I've updated the description to detail what needs to be done.

I have made the 3 changes mentioned above. The resulting image is 320px wide but unfortunately the text box doesn't expand along with it to match it. Shall I/ Do we need to change some values for that as well? I have added the resulting image here.

@Smarita: That's mibad. There's another constant that needs changing in resources/ext.popups/styles/ext.popups.core.less here.

@Smarita: I've moved this to Needs More Work on the Readers Web kanban board so that your work and its state is visible to the Readers Web team 🙂

Also, thanks for picking up this task!

@phuedx : Given below are the latest changes I made. Thank you for letting me work on this! I shall commit these changes and submit the PR asap!

In T173434#3626794, @Smarita wrote:

@phuedx : Given below are the latest changes I made. Thank you for letting me work on this!

@Smarita: That looks great. Thanks! /cc @Nirzar

I shall commit these changes and submit the PR asap!

For better or worse, we use Gerrit for code review. The Gerrit tutorial will help you to get set up and to submit your change.

@phuedx Thanks for your guidance, created commit at https://gerrit.wikimedia.org/r/#/c/379962/ . Kindly review it :)

Change 379962 had a related patch set uploaded (by Zoranzoki21; owner: Smarita):
[mediawiki/extensions/Popups@master] Consider using more common image sizes for Page previews

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

In T173434#3629171, @gerritbot wrote:

Change 379962 had a related patch set uploaded (by Zoranzoki21; owner: Smarita):
[mediawiki/extensions/Popups@master] Consider using more common image sizes for Page previews

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

@Smarita You have to add and Bug: TXXXXX .. I added now, and gerritbot put message here.

@Zoranzoki21 Thanks for pointing that out! I'll keep that in mind.

In T173434#3629258, @Smarita wrote:

@Zoranzoki21 Thanks for pointing that out! I'll keep that in mind.

Your welcome

@Jdlrobson Can you please let me know what else could be added so that I may do so?

@Smarita I've written my feedback on your patch but I can copy here. Hope this helps!:

Patch Set 3: Code-Review-1
There appears to be some test failures as a result of this change
18:26:10   Failed Tests:   There were 3 failures
18:26:10 
18:26:10     x Image SVG width is correct. Height smaller than the predefined height (200)., expected: 303, got: "323", test: createThumbnail - landscape image element, module: ext.popups#renderer, source: at /home/jenkins/workspace/mwgate-npm-node-6-jessie/tests/node-qunit/ui/renderer.test.js:537:10
18:26:10     x Image SVG width is correct. Height bigger than the predefined height (200)., expected: 303, got: "323", test: createThumbnail - landscape image element, module: ext.popups#renderer, source: at /home/jenkins/workspace/mwgate-npm-node-6-jessie/tests/node-qunit/ui/renderer.test.js:537:10
18:26:10     x Layout is correct. Both X and Y are flipped., expected: {
You can replicate these locally by running
> npm install
> npm run test:node
Apart from this the patch looks good to go - I've tested locally.
Almost there! Keep at it you are doing great! :)

Thanks @Jdlrobson ! Will work on the changes ASAP.

For some reason, I didn't receive any notifications from the bot regarding the comments/activity being posted on Gerrit (I'll look into that as well :)

@Jdlrobson @phuedx It passes now :). Kindly do review it. Sorry for delaying this change so much. I was caught up with another issue.

Reference: https://gerrit.wikimedia.org/r/379962

Per T173434#3649291.

Change 379962 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Consider using more common image sizes for Page previews

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

This will also need QA/sign off from @Nirzar.

Looks good to me. Images appear slightly bigger on Beta cluster. I'm not sure if this also effects the 'tall' images, but they look slightly different as well.

Beta	Production

@Nirzar, can you please move to sign-off when you're done?

looks good to me too. BONUS: more content