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

Jwoo/explicit image size uwp #1373

Merged
merged 20 commits into from
Apr 30, 2018
Merged

Jwoo/explicit image size uwp #1373

merged 20 commits into from
Apr 30, 2018

Conversation

jwoo-msft
Copy link
Member

@jwoo-msft jwoo-msft commented Apr 12, 2018

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.

  • If only one dimension is given, the given dimension is explicitly set, and the missing dimension will be adopted to keep the aspect ratio of the image.
  • takes unsigned integer followed by unit; currently only support pix ("px").

@jwoo-msft jwoo-msft added the Platform-XAML Bugs or features related to Xaml Renderer label Apr 13, 2018
@jwoo-msft jwoo-msft force-pushed the jwoo/explicit-image-size-uwp branch from 67d6f62 to d67fe60 Compare April 20, 2018 18:30
UINT32 explicitWidth = 0, explicitHeight = 0;
THROW_IF_FAILED(adaptiveImage->get_Width(&explicitWidth));
THROW_IF_FAILED(adaptiveImage->get_Height(&explicitHeight));
bool isAspectRaitioNeeded = (explicitWidth && explicitHeight);
Copy link
Contributor

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)
Copy link
Contributor

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",
Copy link
Contributor

@RebeccaAnne RebeccaAnne Apr 25, 2018

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

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks


In reply to: 184108904 [](ancestors = 184108904)

@@ -0,0 +1,45 @@
{
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

sure


In reply to: 184109786 [](ancestors = 184109786)

@@ -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);
Copy link
Contributor

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

@@ -0,0 +1 @@
{"HostConfigHash":"d173aab","CardHash":"7ab11e1","Error":null}
Copy link
Contributor

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"
Copy link
Contributor

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

size == ABI::AdaptiveNamespace::ImageSize::Auto ||
explicitWidth ||
explicitHeight ||
isAspectRatioNeeded)
Copy link
Contributor

@RebeccaAnne RebeccaAnne Apr 30, 2018

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

Copy link
Contributor

@RebeccaAnne RebeccaAnne left a comment

Choose a reason for hiding this comment

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

:shipit:

@jwoo-msft jwoo-msft merged commit 2994911 into master Apr 30, 2018
@jwoo-msft jwoo-msft deleted the jwoo/explicit-image-size-uwp branch April 30, 2018 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform-XAML Bugs or features related to Xaml Renderer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants