-
Notifications
You must be signed in to change notification settings - Fork 543
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
Jwoo/explicit image size uwp #1373
Conversation
67d6f62
to
d67fe60
Compare
UINT32 explicitWidth = 0, explicitHeight = 0; | ||
THROW_IF_FAILED(adaptiveImage->get_Width(&explicitWidth)); | ||
THROW_IF_FAILED(adaptiveImage->get_Height(&explicitHeight)); | ||
bool isAspectRaitioNeeded = (explicitWidth && explicitHeight); |
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.
Raitio [](start = 21, length = 6)
Spelling (Ratio) #Closed
@@ -1480,9 +1485,15 @@ AdaptiveNamespaceStart | |||
ComPtr<IShape> ellipseAsShape; | |||
THROW_IF_FAILED(ellipse.As(&ellipseAsShape)); | |||
// Set Auto, None, and Stretch to Stretch_UniformToFill. An ellipse set to Stretch_Uniform ends up with size 0. | |||
if (size == ABI::AdaptiveNamespace::ImageSize::None || | |||
if (isAspectRaitioNeeded) |
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.
Could you add a test file to samples\Tests that demonstrates this and run the test app? #Closed
"type": "Image", | ||
"url": "http://adaptivecards.io/content/cats/1.png", | ||
"size": "small", | ||
"width": "50px", |
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.
[](start = 0, length = 24)
Fix the spacing. I believe we're using tabs in the json examples rather than spaces.
Also, this probably shouldn't go here (v1.0 folder) because it's not a v1.0 feature. We could start a new folder, but i'm not sure we've locked on what we're calling this version. Or just stick it in Test for now until we sort out what we want to do with examples for the next version. #Resolved
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.
@@ -0,0 +1,45 @@ | |||
{ |
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.
Can we add a person type example to this card as well to exercise the ellipse code path? #Closed
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.
@@ -1525,6 +1536,11 @@ AdaptiveNamespaceStart | |||
SetImageOnUIElement(imageUri.Get(), xamlImage.Get(), resourceResolvers.Get()); | |||
THROW_IF_FAILED(xamlImage.As(&frameworkElement)); | |||
|
|||
if(isAspectRaitioNeeded) | |||
{ | |||
xamlImage->put_Stretch(Stretch::Stretch_Fill); |
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.
[](start = 62, length = 15)
[nit] remove trailing spaces #Closed
…daptiveCards into jwoo/explicit-image-size-uwp
@@ -0,0 +1 @@ | |||
{"HostConfigHash":"d173aab","CardHash":"7ab11e1","Error":null} |
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.
Please remove these files generated from the 1.0 version #Closed
"size": "large", | ||
"style": "person", | ||
"width": "40px", | ||
"height": "80px" |
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.
In this one it looks like the ellipse changes size but the picture itself doesn't scale. Is that the desired behavior? #Closed
…/explicit-image-size-uwp
size == ABI::AdaptiveNamespace::ImageSize::Auto || | ||
explicitWidth || | ||
explicitHeight || | ||
isAspectRatioNeeded) |
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.
isAspectRatioNeeded [](start = 16, length = 19)
Don't need this, if this is true, explicitWidth and explicitHeight will already be true. #Resolved
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.
UWP changes
I assumed that UWP content mode of displaying image is simmilar to Scale-To-AspectRatio of iOS, so I set only the dimension that has unsigned non-zero values when one them is zero.Allows image size to be explicitly set given size (width and height) even if doing so will distort the image.