Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

layout delay meter: use own viewport-observer #30970

Closed
wants to merge 1 commit into from

Conversation

samouri
Copy link
Member

@samouri samouri commented Nov 3, 2020

summary
Teach the LayoutDelayMeter to use its own ViewportObserver instead of relying on Resources to call enterViewport()

Partial for #30620

}, this.win_)
);
}
viewportObservers.get(this.win_).observe(this.element_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that's missing is the unobserve when the element is disconnected. Overall the modal of the layout-delay-meter is super similar to the loading indicator. But I refactored it slightly different. And IMHO that looked a bit better. In that model this would look like this:

  1. There's a single "instance" per window that deals with layout-delay-meter for any element in that window. Let's call it LoadDelayTracker.
  2. CE only needs to call track(this) and untrack(this) on the LoadDelayTracker.
  3. The LoadDelayTracker would instantiate a single instance of InOb for its window.
  4. The LoadDelayTracker would keep some minimal per-element state in a WeakMap.
  5. That's it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea. Its a bigger shift away in code style / a bigger refactor, but I agree it'd be much cleaner! Luckily we can just remove it outright :)

@samouri samouri closed this Nov 3, 2020
@samouri samouri deleted the extract-ldm branch November 3, 2020 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants