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

FallbackImageDirective and resolve known npm security vulnerabilities #136

Conversation

crhistianramirez
Copy link
Contributor

@crhistianramirez crhistianramirez commented Aug 14, 2018

Description

FallbackImageDirective: this directive will listen to image error event and replace the failed image with a fallback one, I had added it to winmark and thought it might make a good addition here. I had some tests for this but they relied on timeout and were not consistent.

Security Vulnerabilities: npm audit had revealed several known security vulnerabilities that were waiting on this issue to be closed out - karma-runner/karma#2994 after upgrading karma we now have no known security vulnerabilities

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the acceptance criteria on the task so that it can be tested

Crhistian Ramirez added 2 commits August 13, 2018 21:49
this directive will listen to image error event and replace the failed image with a fallback one
@@ -3,6 +3,7 @@
<div class="row align-items-sm-center">
<div class="col-3 p-0">
<img class="img-thumbnail"
appFallbackImage
[src]="lineitem.Product.xp?.primaryImageURL || 'http://placehold.it/300x300'">
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to make sure you know || 'http://placehold.it/300x300'" is still in the src attribute. Is that how you want 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.

Yep. the || 'http://placehold.it/300x300'" will take care of the scenario where there is no image defined at all.

My directive will take effect when an image url is defined, but getting that image fails. Possibly the url is simply incorrect. In that case, on fail, itll get replaced by the placeholder url

@oliverheywood451 oliverheywood451 merged commit 5182dcf into ordercloud-api:development Aug 15, 2018
@crhistianramirez crhistianramirez deleted the fallback-image-directive branch September 5, 2018 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants