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.

Sign up for GitHub

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

Jump to bottom

feat(common): add Image directive skeleton #45627

Merged
merged 1 commit into from Apr 15, 2022
Merged

feat(common): add Image directive skeleton #45627

merged 1 commit into from Apr 15, 2022

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Apr 14, 2022 •
edited

This commit adds Image directive skeleton as well as a set of basic tests.

// cc @kara @pkozlowski-opensource

PR Type

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added 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) labels Apr 14, 2022
@ngbot ngbot bot modified the milestone: Backlog Apr 14, 2022
@AndrewKushnir AndrewKushnir force-pushed the img-dir-skeleton branch 2 times, most recently from 2ca96c7 to edfa78d Compare April 14, 2022 05:22
@AndrewKushnir AndrewKushnir force-pushed the img-dir-skeleton branch 2 times, most recently from 392f54f to 8977e04 Compare April 15, 2022 01:31
@AndrewKushnir AndrewKushnir requested a review from kara April 15, 2022 04:26
@AndrewKushnir AndrewKushnir marked this pull request as ready for review April 15, 2022 04:26
@pkozlowski-opensource
Copy link
Member

Thnx for putting this skeleton together @AndrewKushnir . Left a couple of comments (or more of items to be discussed).

But more importantly I would personally prefer that we don't add all the @Inputs to the directive. I was assuming that we will do a minimal (like really minimal :-)) skeleton first and will keep adding inputs / functionality as we go. This is mostly because:

  • I'm not sure what the final scope will be;
  • I would prefer to have as little "noise" as possible

This is would be my personal preference, TBD

kara
kara reviewed Apr 15, 2022
View reviewed changes
Copy link
Contributor

@kara kara left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for putting this together! Overall, it looks great, but requesting that we temporarily remove some of the features until we can discuss the MVP design more

packages/common/src/directives/index.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_image.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_image.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_image.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_image.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir force-pushed the img-dir-skeleton branch 3 times, most recently from 4d053e7 to c72d14e Compare April 15, 2022 22:14
@AndrewKushnir
Copy link
Contributor Author

@kara @pkozlowski-opensource thanks for the feedback. I've applied the necessary changes, will get CI to a "green" state and proceed with merging this PR to the image-directive branch.

pkozlowski-opensource
pkozlowski-opensource reviewed Apr 15, 2022
View reviewed changes
packages/common/test/directives/ng_image_spec.ts
setupTestingModule({imageLoader});

const template =
'<img raw-src="path/img.png" [loader]="cmpImageLoader" width="150" height="50">';
Copy link
Member

Choose a reason for hiding this comment

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

Question: do we actually want to specify a loader on the per-instance basis?

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 question: I think this gives a nice flexibility, but I'm not sure if it'd be useful in practice. I'll keep it for now and if the decision is to drop it, I can create a followup PR.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, I would prefer that, in the future, we drop / don't include parts that we are unsure about, just to reduce "noise".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree :)

It was a part of the doc and since we support image loaders via a token, I consider that extra option to be inline with the overall direction, so I decided to leave it (and I'm happy to update the code if we decide that it's not useful at all).

@AndrewKushnir
feat(common): add Image directive skeleton
5dc6d79 
This commit adds Image directive skeleton as well as a set of basic tests.
@AndrewKushnir AndrewKushnir merged commit 0f9d58b into angular:image-directive Apr 15, 2022
AndrewKushnir added a commit that referenced this pull request Apr 16, 2022
@AndrewKushnir
feat(common): add Image directive skeleton (#45627)
4c959d6 
This commit adds Image directive skeleton as well as a set of basic tests.
AndrewKushnir added a commit that referenced this pull request Apr 20, 2022
@AndrewKushnir
feat(common): add Image directive skeleton (#45627)
3fae0e6 
This commit adds Image directive skeleton as well as a set of basic tests.
AndrewKushnir added a commit that referenced this pull request Apr 20, 2022
@AndrewKushnir
feat(common): add Image directive skeleton (#45627)
779bf82 
This commit adds Image directive skeleton as well as a set of basic tests.
AndrewKushnir added a commit that referenced this pull request Apr 26, 2022
@AndrewKushnir
feat(common): add Image directive skeleton (#45627)
7ac9fb5 
This commit adds Image directive skeleton as well as a set of basic tests.
AndrewKushnir added a commit that referenced this pull request May 3, 2022
@AndrewKushnir
feat(common): add Image directive skeleton (#45627)
1d86657 
This commit adds Image directive skeleton as well as a set of basic tests.
khempenius pushed a commit to khempenius/angular that referenced this pull request May 10, 2022
@AndrewKushnir @khempenius
feat(common): add Image directive skeleton (angular#45627)
0625540 
This commit adds Image directive skeleton as well as a set of basic tests.
khempenius pushed a commit to khempenius/angular that referenced this pull request May 10, 2022
@AndrewKushnir @khempenius
feat(common): add Image directive skeleton (angular#45627)
e6baa45 
This commit adds Image directive skeleton as well as a set of basic tests.
AndrewKushnir added a commit that referenced this pull request May 13, 2022
@AndrewKushnir
feat(common): add Image directive skeleton (#45627)
1e0ac75 
This commit adds Image directive skeleton as well as a set of basic tests.
@angular-automatic-lock-bot
Copy link

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 May 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource left review comments

@kara kara kara left review comments

Assignees
No one assigned
Labels
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
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AndrewKushnir @pkozlowski-opensource @kara