Page MenuHomePhabricator

upstream the isElementInViewport function
Closed, ResolvedPublic5 Story Points

Description

We've been using this function in different extensions. Let's upstream it.

Acceptance criteria:

  • Upstream the code
  • Remove local implementations from MobileFrontend
  • Remove local implementation from QuickSurveys

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bmansurov raised the priority of this task from to Needs Triage.Jan 21 2016, 6:31 PM
bmansurov updated the task description. (Show Details)
bmansurov added a subscriber: bmansurov.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptJan 21 2016, 6:31 PM
bd808 triaged this task as Normal priority.Jan 25 2016, 6:10 PM
bd808 added a subscriber: bd808.
bmansurov updated the task description. (Show Details)Jan 27 2016, 5:07 PM
bmansurov set Security to None.
jhobs added a subscriber: jhobs.

Anyone have a good idea of where specifically this should live in core (a.k.a. is there a general place for utility functions)? Haven't quite touched all the corners of that behemoth yet. ;)

mediawiki.util.js maybe?

Change 267058 had a related patch set uploaded (by Jhobs):
Upstream isElementInViewport from MobileFrontend

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

Have commented that I think this requires a little more thought and wider input. I suspect this will carry over to next sprint which is fine - upstreaming takes time.

phuedx added a subscriber: phuedx.Jan 29 2016, 11:05 AM

I have also added my two $monetaryUnits.

Yeah, I saw, thanks for your comments. I'm going to let the discussion ride out a bit (chiming in myself as well of course) before uploading any new patch sets.

Incidentally, suspicion that this may happen is also why I haven't written the MF & Gather patches yet. ;)

This comment was removed by jhobs.

I don't know why, but it posted my previous comment twice... hence the now-deleted one.

@jhobs any updates on this?

@jhobs what @bmansurov said on e-mail - you should fix it up and not wait for more reviews.

I think my phrasing in the email was poor. I was not suggesting halting development, I simply meant I hope more people look at it soon in addition and it gets +2'd quickly after all the minors are taken care of.

@jhobs, sorry for misunderstanding.

Change 269914 had a related patch set uploaded (by KartikMistry):
New upstream release

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

Change 269915 had a related patch set uploaded (by KartikMistry):
New upstream release

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

Change 269916 had a related patch set uploaded (by KartikMistry):
apertium-dan-nor: New upstream release

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

Is there any way to disconnect those 3 patches by KartikMistry from this task? They're not related at all (and appear to be duplicates).

To avoid confusion with the above unrelated patches the patch that needs review is https://gerrit.wikimedia.org/r/267058

Jdlrobson moved this task from Backlog to Tasks on the MobileFrontend board.Feb 18 2016, 6:41 PM

Was some useful input from @Krinkle
Now waiting on a follow up from @jhobs

QUnit is complaining.

I've run the test suite locally several times and can't find any issues. It appears to me that Jenkins is failing.

FYI php notice in https://integration.wikimedia.org/ci/job/mediawiki-extensions-qunit/32297/consoleFull

(Always read the logs)
rebase or recheck should fix

FYI php notice in https://integration.wikimedia.org/ci/job/mediawiki-extensions-qunit/32297/consoleFull
(Always read the logs)
rebase or recheck should fix

I read the logs, but it was the same in PS 12 & 13 even though I rebased in 13, so I too thought maybe a recheck would work, to no avail. Just rebased again, hopefully it'll work this time.

The error that keeps being thrown is

PHP Notice: Cannot find site jenkins_u0_mw in sites table [Called from Wikibase\Client\WikibaseClient::newSiteGroup in /mnt/jenkins-workspace/workspace/mediawiki-extensions-qunit/src/extensions/Wikidata/extensions/Wikibase/client/includes/WikibaseClient.php at line 597] in /mnt/jenkins-workspace/workspace/mediawiki-extensions-qunit/src/includes/debug/MWDebug.php on line 300

which refers to a file that I did not touch in the patch. I have no idea how to proceed from here.

Change 267058 merged by jenkins-bot:
Upstream isElementInViewport from MobileFrontend

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

This cannot be considered done until the core function is used by MobileFrontend and friends and the code in those extensions is cleaned up.

Jdlrobson updated the task description. (Show Details)Mar 4 2016, 12:40 AM
Krinkle removed a subscriber: Krinkle.Apr 4 2016, 11:36 PM

Change 282194 had a related patch set uploaded (by Bmansurov):
Remove the isElementInViewport function

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

Change 282196 had a related patch set uploaded (by Bmansurov):
Remove the isElementInViewport function

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

QuickSurveys and RelatedArticles have been covered in the above two patches. MobileFrontend has already been taken care of.

dr0ptp4kt set the point value for this task to 5.Apr 11 2016, 5:49 PM

Change 282194 merged by jenkins-bot:
Remove the isElementInViewport function

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

ori added a subscriber: ori.Apr 11 2016, 6:22 PM

Per T130849, isElementInViewport is an anti-pattern, because it forces a layout. I'd really rather see it removed entirely rather than standardized.

Per T130849, isElementInViewport is an anti-pattern, because it forces a layout. I'd really rather see it removed entirely rather than standardized.

It's worth noting that the article you point to recommends "All calls to get any calculated dimension from the DOM should be cached or avoided."
We could certainly do the former and I would argue we can do it far more easily with the mediawiki.viewport library.

That said, this is an essential component of the lazy loading images solution. I'm curious to whether you know of an alternative for achieving lazy loading images without being able to test whether an image has been scrolled into view.

ori added a comment.Apr 11 2016, 9:07 PM

Per T130849, isElementInViewport is an anti-pattern, because it forces a layout. I'd really rather see it removed entirely rather than standardized.

It's worth noting that the article you point to recommends "All calls to get any calculated dimension from the DOM should be cached or avoided."
We could certainly do the former and I would argue we can do it far more easily with the mediawiki.viewport library.
That said, this is an essential component of the lazy loading images solution. I'm curious to whether you know of an alternative for achieving lazy loading images without being able to test whether an image has been scrolled into view.

Right -- that's a good point. The use-cases I was thinking of were feature usage analytics.

Change 282196 merged by jenkins-bot:
Remove the isElementInViewport function

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

Given the discussion on this ticket and T130849, the Intersection Observer spec is definitely worth tracking.

Change 269914 merged by Alexandros Kosiaris:
apertium-nob: New upstream release

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