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/ios columnsetupdate #5296

Merged
merged 27 commits into from
Jan 23, 2021
Merged

Jwoo/ios columnsetupdate #5296

merged 27 commits into from
Jan 23, 2021

Conversation

jwoo-msft
Copy link
Member

@jwoo-msft jwoo-msft commented Jan 20, 2021

Related Issue

Fixed #5096, Fixed #4353, Fixed #3677

Description

Fixed numerous issues with Images. Fixed FactSet stretch issues. Updated the sample app not to render when the table view is in a collapsed state.
Before Fix
Simulator Screen Shot - iPhone Xʀ - 2021-01-19 at 18 30 53
After Fix
image

Before Fix
simulator_screenshot_6C2B8E6E-530E-4B3E-A7D2-CC8FEFA05302

After Fix
image

Before Fix
image

After Fix
image

Before Fix
image

After Fix
image

Before Fix
image

After Fix
image

Before Fix
The size of the Image in the center is "stretch"
image

After Fix
The size of the Image in the center is "stretch"
image

Before Fix
image

After Fix
image

How Verified

How you verified the fix, including one or all of the following:

  1. New Unit Test is added
  2. All of the unit tests are run.
  3. All of the relevant sample cards are tested.
Microsoft Reviewers: Open in CodeFlow

jwoo-msft and others added 20 commits December 16, 2020 21:04
* expression library rc to 4.11.0

* Add expression library to site footer scripts
With our 20.12 release we are fixing a long running bug, where we were shipping our binary packages under the MIT license inadvertently. The MIT license only applies to open source and not shipping binaries which are now governed by our new End User License Agreement for AdaptiveCards.
@ghost
Copy link

ghost commented Jan 20, 2021

Hi @jwoo-msft. Thanks for helping make the AdaptiveCards JS renderer + tooling better. As additional verification, once the JS build succeeds, please go to the test site to test out your website/designer changes.

@ghost ghost removed the Needs: Author Feedback label Jan 22, 2021
Copy link
Member

@paulcam206 paulcam206 left a comment

Choose a reason for hiding this comment

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

looks good, but found a few minor nits 👍

Comment on lines 146 to 147
} else if (self.imageProperties.acrImageSize == ACRImageSizeStretch && frameSize.width != self.imageProperties.contentSize.width && _imageView.image) {
if (_imageView.image) {
Copy link
Member

Choose a reason for hiding this comment

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

we're checking _imageView.image twice :)

// apply now if image is ready, otherwise wait until it is loaded
applyBackgroundImageConstraints(backgroundImage.get(), imgView, img);
applyBackgroundImageConstraints(backgroundImage.get(), imgView, imgView.image);
Copy link
Member

Choose a reason for hiding this comment

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

question here -- is applyBackgroundImageConstraints always passed a UIImageView that has a valid .image? should applyBackgroundImageConstraints just use the image property rather than having it passed in as a parameter?

Copy link
Member Author

@jwoo-msft jwoo-msft Jan 23, 2021

Choose a reason for hiding this comment

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

your question is a valid one. There is a case SDK can have a UIImageview and UIImage separately, but such cases are rare. I will look into this issue when I refactor the background image rendering which is due soon.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good 👍

Copy link
Member

@paulcam206 paulcam206 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 935b7da into main Jan 23, 2021
@jwoo-msft jwoo-msft deleted the jwoo/ios-columnsetupdate branch January 23, 2021 01:15
rankush pushed a commit to rankush/AdaptiveCards that referenced this pull request May 8, 2024
* Updated ImageView constraints

* updated intrinsic content size

* reverted build optimazation settings

* [Site] Fix missing expression library dep (microsoft#5194)

* expression library rc to 4.11.0

* Add expression library to site footer scripts

* Updating Readme to reflect binary license updates

With our 20.12 release we are fixing a long running bug, where we were shipping our binary packages under the MIT license inadvertently. The MIT license only applies to open source and not shipping binaries which are now governed by our new End User License Agreement for AdaptiveCards.

* Quick verbiage change in the EULA section

* More license terms explanation verbiage

* [.Net HTML]fix IgnoreDefaultStringEnumConverter NullReference issue (microsoft#4833)

## Related Issue
Fixes microsoft#4831

## Description
Avoid NullReferenceException while the enum is null in JSON string
![Issue](https://user-images.githubusercontent.com/3928402/94417848-2f57f780-01b3-11eb-88d7-38eb24e02457.png)

* columnset alignment to fill, and added spacer for factset

* work in progress

* work in progress

* Mostly work except background images

* removed the wrapperview

* refactoring

* refactoring complete // need unit tests and optimazation

* fixed stretch issues

* Finished fixing small/medium/large images sizes

* Added UnitTests

* cleaned up the code

* complete

* fixed ci failures

* revert project file changes

* chaged made after review comments

Co-authored-by: Risheek Rajolu <risheekrr@gmail.com>
Co-authored-by: Shalini Joshi-MSFT <shalinij@microsoft.com>
Co-authored-by: Tony <lxtx.ljb@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants