On Forem (the open source software that powers DEV, etc.) we have these display units with customizable content an admin can set.

As you can see, this contains an image... which will need to load asynchronously as all images do in HTML. If height and width are set, then the browser will save the space for the image to take up once it fully loads. If these values are not set (and no custom styles are set), this will cause the page content to "jump", which is not good for UX, and is penalized by Google as "cumulative layout shift"...

Fixing Cumulative Layout Shift

Ben Halpern ・ Jan 19 '21

#webdev #css #javascript #html

These images are user-generated-content, and we do not want to ask admins to manually declare the image size. They may get it wrong, but even if they get it right, it's not exactly the best use of their time if the application can handle it for them.

Detecting image dimensions is not rocket science, but you don't want to reinvent the wheel or do it inefficiently. Enter FastImage.

sdsykes / fastimage

FastImage finds the size or type of an image given its uri by fetching as little as needed

FastImage

FastImage finds the size or type of an image given its uri by fetching as little as needed

The problem

Your app needs to find the size or type of an image. This could be for adding width and height attributes to an image tag, for adjusting layouts or overlays to fit an image or any other of dozens of reasons.

But the image is not locally stored - it's on another asset server, or in the cloud - at Amazon S3 for example.

You don't want to download the entire image to your app server - it could be many tens of kilobytes, or even megabytes just to get this information. For most common image types (GIF, PNG, BMP etc.), the size of the image is simply stored at the start of the file. For JPEG files it's a little bit more complex, but even so you do…

View on GitHub

FastImage inspects the image and and read the minimal information needed for the task, in this case determining dimensions. In Forem, we already had in place a process for proxying the images so that they are served by the appropriate CDN instead of direct from the upload source, so adding FastImage was a matter of adding this as an option to that step in the process.

I added an option in our image manipulation step called synchronous_detail_detection which, if true, takes the time to inspect images and output the proper attributes.

Doing this synchronously is the least complicated way to do this, and I felt like it was appropriate for this functionality given that it is only used by admins. When we use FastImage to detect details on posts, which can be created by any member of the site, it makes more sense to do this inspection asynchronously to ensure a post with 15 images can successfully save without timing out.

The code for the functionality described is all visible in this pull request, where I made several changes to clean up this particular functionality.

Fix inconsistencies with DisplayAd #14789

benhalpern posted on Sep 22, 2021

What type of PR is this? (check all applicable)

[x] Refactor
[x] Feature
[x] Bug Fix
[ ] Optimization
[ ] Documentation Update

Description

At the core of this PR, is adding back in sanitation so that we allow for a consistent, safe interface for creating DisplayAd content. It's currently awkward and fairly unsafe to use this functionality for many creators. This fixes things.

Therefore removes this code:

# Temporarily disable the sanitisation in order to launch the SheCoded Campaign. # TODO: find an alternate solution.

I think the core purpose of this is well in the rearview for Forem and DEV needs.

This does remove the capacity for inline styles (not immediately breaking existing stuff, but will on re-save of anything), but adds the same wrapping class to the left-hand ads as the right. This should be a big win for visual consistency while still providing a lot of flexibility as far as how admins might choose to use this functionality.

This also fixes the issue of DisplayAdEvent tracking only working for the left ad and not the right ad. This now works across the board, meaning we track impressions and clicks.

We already use a functionality called for_display to determine which ad to display in order to reward top performing display units on average, but without consistent and complete implementation of the click tracking, it is hard for admins to understand how to use this functionality and we can't really document it properly.

I also added one new location: left_sidebar_2 which will now allow admins to set two units on the left if they choose to, and they all work with the same consistency.

This also uses FastImage to detect image aspect ratios so they display without the page jumping when rendered without being cached. E.g. like this:

https://user-images.githubusercontent.com/3102842/134372470-ff602677-60b2-4fea-97e5-fbea214c4953.mov

All in all this should provide clarity and stability to allow admins to make more use of these features. We sure will be able to take advantage of this on DEV.

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, a note on the devices and browsers this has been tested on, as well as any relevant images for UI changes.

UI accessibility concerns?

If your PR includes UI changes, please replace this line with details on how accessibility is impacted and tested. For more info, check out the Forem Accessibility Docs.

Added/updated tests?

[x] Yes
[ ] No, and this is why: please replace this line with details on why tests have not been included
[ ] I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the development process, or any of our internal teams? If so, please note how you will share this change with the people who need to know about it.

This will provide an opportunity to add more clear and detailed docs of how this functionality works.

[x] I plan to make a PR into to the admin docs myself when this is merged.