Skip to content

feat(common): add loaders for Cloudinary and ImageKit providers - #46277

Merged
pkozlowski-opensource merged 3 commits into
angular:image-directivefrom
khempenius:additional_loaders
Jun 9, 2022
Merged

pkozlowski-opensource merged 3 commits into
angular:image-directivefrom
khempenius:additional_loaders

Conversation

@khempenius

Copy link
Copy Markdown
Contributor

PR Type

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • Yes
  • No

@pullapprove
pullapprove Bot requested review from alxhub and devversion June 7, 2022 00:50
@khempenius
khempenius force-pushed the additional_loaders branch from dab8867 to 5bcbdcd Compare June 7, 2022 00:54
@AndrewKushnir AndrewKushnir added PullApprove: disable target: feature This PR is targeted for a feature branch (outside of main and semver branches) labels Jun 7, 2022
@AndrewKushnir
AndrewKushnir requested review from AndrewKushnir and kara and removed request for alxhub and devversion June 7, 2022 00:56
@AndrewKushnir AndrewKushnir added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 7, 2022

@pkozlowski-opensource pkozlowski-opensource left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The change LGTM overall but we start to have code duplication between various loaders and I would love to see the common logic extracted before this PR gets merged.

@khempenius
khempenius force-pushed the additional_loaders branch from 5bcbdcd to 4dc26b4 Compare June 7, 2022 14:32

@kara kara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, minor nits below

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: path error seems like it can be shared if the name of the loader is also passed as an arg

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wrote slightly different error messages for the Cloudinary & Imgix loaders (which is why I didn't move more of this into loader_utils). I considered passing the whole error message as an argument but that felt awkward IMO.

@khempenius
khempenius force-pushed the additional_loaders branch from 4dc26b4 to ea0076e Compare June 7, 2022 21:32
@alxhub alxhub added the area: common Issues related to APIs in the @angular/common package label Jun 8, 2022
@ngbot ngbot Bot added this to the Backlog milestone Jun 8, 2022
khempenius and others added 2 commits June 8, 2022 17:11
This commit adds loaders for cloudinary and imagekit.
…e ensurePreconnect option

This commit modifies the provideCloudinaryLoader and provideImageKitLoader functions to support an additional ensurePreconnect option, similar to the Imgix loader functions.
@AndrewKushnir AndrewKushnir changed the title Add loaders for Cloudinary and ImageKit feat(common): add loaders for Cloudinary and ImageKit providers Jun 9, 2022
This commit refactors the code to avoid an extra utils file and instead, all extra helpers are moved to a common location.
@khempenius khempenius mentioned this pull request Jun 9, 2022
1 of 2 tasks

@pkozlowski-opensource pkozlowski-opensource left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@pkozlowski-opensource
pkozlowski-opensource merged commit fbe005e into angular:image-directive Jun 9, 2022
@angular-automatic-lock-bot

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package PullApprove: disable target: feature This PR is targeted for a feature branch (outside of main and semver branches)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants