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

Bug 1531933 – Use proper image sizes #4885

Merged
merged 42 commits into from Apr 11, 2019

Conversation

@gvn
Copy link
Collaborator

commented Apr 5, 2019

Testing:

  • Try some layout variants (recommend: 66-beta-2-complex, basic, dev-test-all).
  • Ensure no images break (caveat: since there's a CDN and a image processing service middle tier you may see something slow to load if you're the first to hit it...)

@gvn gvn requested review from ScottDowne and punamdahiya Apr 5, 2019

@gvn

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 8, 2019

Going to do a little refactoring...

@ScottDowne

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

That went together fairly fast!

First thing I noticed (unless I missed it) you probably need something similar to this:

  componentWillUnmount() {
    if (this._handleIntersect && this.impressionObserver) {
      this.impressionObserver.unobserve(this.refs.impression);
    }
    if (this._onVisibilityChange) {
      this.props.document.removeEventListener(VISIBILITY_CHANGE_EVENT, this._onVisibilityChange);
    }
  }

I pulled that from our impression code, so probably want to adapt it a bit.

Also I'm still looking at your pr

@ScottDowne

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

If I open a new tab using the + button, it doesn't actually render images. I think this has to do with either prerendered images, or document.visibilityState === visible or a combination of both?

I know when me and Nan were working on the Impressions stuff, we had to use a bit of care with the visibility stuff.

Locally I removed the document.visibilityState === visible check, and the + is now working as expected.

I wonder if we should sanity check this with Nick Chapman?

@ScottDowne

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

Finally, I think you need to go through the array in element in onSeen because sometimes the first item is an unseen intersection, and the last one is the seen one? Not exactly sure how that works, but an example can be seen here: https://github.com/mozilla/activity-stream/blob/master/content-src/components/DiscoveryStreamImpressionStats/ImpressionStats.jsx#L138

Notice the entries.some. I think it has to do if you scroll or page load, it doesn't fire the events off async as the browser updates, but does it on it's own clock, so if two events come through in that time, it sends back both entries, and you need to check for any. But not sure about that... From my testing, I am getting back cases where I have an array of two items, the first element[0].isIntersecting is false and the second element[1].isIntersecting is true. In these cases, no images render.

@gvn gvn removed the request for review from punamdahiya Apr 9, 2019

@gvn

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 9, 2019

Looks like I'll need to add some test coverage for DSImage.jsx now that our requirements are higher.

@gvn

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 9, 2019

If I open a new tab using the + button, it doesn't actually render images. I think this has to do with either prerendered images, or document.visibilityState === visible or a combination of both?

Taking out that check fixed opening new new tabs. 👍

gvn added some commits Apr 9, 2019

@gvn gvn merged commit 400eacc into mozilla:master Apr 11, 2019

2 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.