-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
ARCH-184 added images element and all necessary files to apply image defaults #175
Conversation
Cc: @bixgomez |
@@ -0,0 +1 @@ | |||
<img src="{{ src }}" alt="{{ alt }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plan to use this template or is it just example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an example. The mixin can be used as needed, but this was mostly to provide sensible defaults and provide an example for the pattern library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I was going to suggest adding an {{ attributes }}
variable so that developers could add additional attributes to the element tag.
eg: <img src="{{ src }}" alt="{{ alt }}" {{ attributes }}>
But perhaps we don't to dismay future developers from using this template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even more, do we even need an example for something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example page may be needed in the future. Perhaps for now, if we are not intending the developer to use the .twig file we put the markup directly in to the sass comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up and moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is fine to me.
@sherakama there is an open ticket in a current sprint that ends soon that this works relates to. If this looks good (or needs more work), let me know. Hoping to get this merged. |
READY FOR REVIEW
Summary
Needed By (Date)
Urgency
Steps to Test
Affected Projects or Products
Associated Issues and/or People
See Also