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

Ignore images that take the full page viewport #81

Merged
merged 1 commit into from
Sep 30, 2021
Merged

Conversation

npm1
Copy link
Collaborator

@npm1 npm1 commented Sep 29, 2021

Fixes #73


Preview | Diff

@npm1 npm1 requested a review from yoavweiss September 29, 2021 15:31
@npm1
Copy link
Collaborator Author

npm1 commented Sep 29, 2021

A few things:

  • Note sure why ipr is complaining?
  • The way I used to get the top-level document is a bit convoluted, not sure if there is an easier way
  • CSSOM does not really give a lot of details about how this viewport sizes are computed, see for example https://drafts.csswg.org/cssom-view/#dom-element-clientwidth

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

  • Note sure why ipr is complaining?

I suspect that was a glitch. The bots seem to like you now

  • The way I used to get the top-level document is a bit convoluted, not sure if there is an easier way

Suggested a simplification, which can work if my assumptions are correct

index.bs Show resolved Hide resolved
Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

LGTM

@npm1 npm1 merged commit e57f8ab into main Sep 30, 2021
@npm1 npm1 deleted the ignore-full-viewport branch September 30, 2021 16:57
noamr added a commit that referenced this pull request Nov 13, 2024
Instead of initializing `renderTime`, take it from the paint timing info
as propagated from paint timing.

See w3c/paint-timing#62 and wicg/element-timing/#81
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.

Exclude full viewport images
2 participants