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

Support enabling blurhash explicitly #373

Merged
merged 3 commits into from
Nov 30, 2021

Conversation

LevelbossMike
Copy link
Contributor

Thanks for this great addon!

Currently, the addon only supports using blurhash when images are optimized with this technique at build time. An app that I'm working on wants to show a lot of images but has no static images to prepare with this technique at build time.

This PR allows setting the usesBlurhash - option explicitly which will include the necessary things to work with blurhash even if no other static images should be rendered with this technique.

I'm not sure how to test this behavior (i.e. changing config data) but would be happy to add some tests if you can give me pointers on how to do so.

@simonihmig simonihmig self-requested a review November 30, 2021 09:29
Copy link
Owner

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

Besides the inline comments, can you shortly explain how you use this, so that I can better understand your use case. When you are not using local images, do you have a custom "provider" helper that exposes the blurhash data to the image component?

README.md Outdated
@@ -254,6 +254,7 @@ Configuration of the addon is done in your `ember-cli-build.js`:
let app = new EmberAddon(defaults, {
'responsive-image': {
fingerprint: true,
usesBlurhash: false,
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should omit this here? In general I think it would be better to rely on the default behavior, as your case is rather special I think. And folks might just copy paste this without thinking about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - updated

README.md Outdated
@@ -281,6 +282,7 @@ let app = new EmberAddon(defaults, {
* **fingerprint**: Can be used to enable/disable fingerprinting of the generated image files. In most cases you can omit
setting this explicitly, as it will follow whatever you have set under the main `fingerprint` options (used by the `broccoli-asset-rev` addon),
with the default being to enable fingerprinting only in production builds.
* **usesBlurhash**: make the addon support blurhash explicitly. This is useful when you aren't prebuilding image data but want to use blurhash for dynamic image data included in API responses.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* **usesBlurhash**: make the addon support blurhash explicitly. This is useful when you aren't prebuilding image data but want to use blurhash for dynamic image data included in API responses.
* **usesBlurhash**: make the addon support blurhash explicitly. This is useful when you aren't prebuilding image data but want to use blurhash for dynamic image data included in API responses. By default it will automatically enable blurhash support when you have configured local images to use blurhash, so in general you shouldn't need to set this.

index.js Outdated
this.options['@embroider/macros'].setOwnConfig.usesBlurhash =
this.usesBlurhash;
this.usesBlurhash =
this.addonOptions.usesBlurhash ||
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder how we should deal with a case where we have local blurhash images, but usesBlurhash is set to false? Currently it would still use blurhash, so setting to false has no effect (due to how || works). We could change this to only check the local images when it is undefined (basically use ?? instead of ||, but node 12 does not support this yet, does it?)

Or should we throw a build-time assertion in that case, basically saying that this is an invalid contradiction?

Copy link
Contributor Author

@LevelbossMike LevelbossMike Nov 30, 2021

Choose a reason for hiding this comment

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

The current behavior of the addon is to automatically turn on blurhash when there are images that should use blurhash. I feel that's the correct behavior and I doubt anybody would want to turn it off blurhash when they have images that are defined to use blurhash.

The PR only makes sure that you can opt-in to use blurhash when there are no pre-build images that are defined to use blurhash - e.g. an app that I'm working on wants to use blurhash in an image search but has no images to precompile.

I feel that allowing users to opt-into including the blurhash library is useful - allowing them to disable it although they apparently have images that should use that mechanism is a potential troll that we might want to avoid?! I also updated the README with your suggested warning that this is not an option you generally would need to set, so this might already be good enough to help people do the right thing.

I.e. I would leave that as is personally but happy to change if you feel like that this is necessary.

Copy link
Owner

Choose a reason for hiding this comment

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

allowing them to disable it although they apparently have images that should use that mechanism is a potential troll that we might want to avoid

Yes, right. That's why I thought we should maybe throw an assertion. Something like that:

const imagesUseBluhash = this.addonOptions.images.some(
      (imageConfig) => imageConfig.lqip && imageConfig.lqip.type === 'blurhash'
    );
if (this.addonOptions.usesBlurhash === false && imagesUseBluhash) {
  throw new SilentError('You cannot disable blurhash support while it is still used in your images config. Please remove any instance of `lqip: { type: 'blurhash' }` from your images config.');
}  

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - I pushed an update.

@LevelbossMike
Copy link
Contributor Author

LevelbossMike commented Nov 30, 2021

Regarding your question of how I'm using this:

I'm working on an app that lets users search for images. Think instagram. And we want to show a preview image and not just have images pop-in when they are loaded. Users will see a lot of images at once in the image search so it's a nicer user experience to show a blurhash preview instead of nothing until the actual image-thumbnail has been loaded.

There's a backend service that calculates the blurhash so search items will contain a blurhash attribute in the search-response. To surface this to the users we have created a custom provider helper that knows how to handle search-items and how to extract the blurhash and thumbnail attributes.

To make this mechanism work I need to tell ember-responsive-image to include the blurhash-library into the application explicitly. There are no images that I need to precompile with blurhash so there wasn't a way to do so before this PR because there was no way to set an option for the addon to include blurhash.js.

@simonihmig
Copy link
Owner

To surface this to the users we have created a custom provider helper that knows how to handle search-items and how to extract the blurhash and thumbnail attributes.

Yeah, that was the piece I wondered about. Really great to see that this provider helper API - that I so far only used for image CDNs - seems to have covered your use case properly it seems! If there is anything that can be improved, then please share your experiences. And whenever that thing you are working is launched and publicly visible, please ping me, I would love to see this in action! 😍

Copy link
Owner

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

Thanks again! Will merge once CI is passing.

@simonihmig simonihmig merged commit fc8619a into simonihmig:master Nov 30, 2021
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.

None yet

2 participants