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

Faster fades on load and progressive display of images #3169

Merged
merged 6 commits into from
Mar 2, 2021

Conversation

AWare
Copy link
Contributor

@AWare AWare commented Feb 15, 2021

What does this change?

We are using a very nice fading of thumbnails and main preview. Sadly, it depends on them being loaded in full. We have imgOps configured to provide progressive JPEGs.

This PR:

  • changes the fading so that it doesn’t depend on an image being fully loaded
  • changes the thumbnail generation, so that they are also progressive JPEGs

Currently, the image fade in on load applies an element style using the
angular digest loop. As this style stuff is done on the element directly
there's no state here for angular to track. I've reorganised things to:

  • make the reveal happen before we do any $q promisey stuff, so no more waiting on the digest loop
  • set a timeout (which ironically will fire in the digest loop) so that after a while we just show the image

How can success be measured?

I think this would work better with #3166 from @paperboyo than just dropping the fade. The app should feel more snappy.

Screenshots

(bottom: current situation)

Viewer:

both

Browser (played at 25% speed for effect):

both_thumb5

Who should look at this?

@guardian/digital-cms

Tested?

  • locally by committer
  • locally by Guardian reviewer
  • on the Guardian's TEST environment

@AWare AWare requested a review from a team as a code owner February 15, 2021 16:54
@paperboyo paperboyo mentioned this pull request Feb 18, 2021
3 tasks
@sihil
Copy link
Contributor

sihil commented Feb 26, 2021

Waht are you thinking about this @AWare. I'm keen not to pick up any more work like this, but if this is close to shippable then it makes sense to realise the value by getting it over the line rather than parking it to rot.

@paperboyo
Copy link
Contributor

This indeed works better than #3166 (see additional explanation/rationale and comparisons with current PROD there). Here are further comparisons. This PR – top, #3166 – bottom:

Viewer:

alextopmatboittom_b

Browser:

alextopmatboittom_browser

@paperboyo paperboyo changed the title faster fades on load Faster fades on load and progressive display of images Mar 1, 2021
Comment on lines +10 to +11
const animationDuration = 200; // ms
const revealAfter = 1500; // ms
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] think this indentation is accidental

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the joys of prettier

Copy link
Contributor

@twrichards twrichards left a comment

Choose a reason for hiding this comment

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

logic LGTM 👍
(i'm in the middle of a rebase of my grid work so I haven't tested it locally)

AWare and others added 6 commits March 2, 2021 12:30
Currently the image fade in on load applies an element style using the
angular digest loop. As this style stuff is done on the element directly
there's no state here for angular to track. I've reorganised things to:
- make the reveal happen before we do any $q promisey stuff, so no more waiting on the digest loop
- set a timeout (which ironically will fire in the digest loop) so that after a while we just show the image

some changes
@prout-bot
Copy link

Seen on auth, usage, cropper, collections, media-api, kahuna, image-loader, image-loader-projection, metadata-editor, leases (created by @AWare and merged by @paperboyo 15 minutes ago) Please check your changes!

ochiengolanga pushed a commit to bbc/grid that referenced this pull request Mar 4, 2021
Faster fades on load and progressive display of images
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants