Added placeholder support #4897
Conversation
ghost
added
the
|
Hi @jwoo-msft. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. #Closed |
|
Hi @jwoo-msft; Thanks for taking action on your previously stale pull request. Resetting staleness. #Closed |
|
Out of curiosity, is this something that was missed on iOS but is available on the pther platforms? #Closed |
source/ios/AdaptiveCards/AdaptiveCards/AdaptiveCards/ACRIBaseCardElementRenderer.h
Outdated
Show resolved
Hide resolved
|
Nit/typo: *cached |
source/ios/AdaptiveCards/AdaptiveCards/AdaptiveCards/ACRImageRenderer.mm
Show resolved
Hide resolved
i don't believe we support placeholder images while the image is loading on other platforms. The usage here is unique to Cisco. They know exactly the size of the image they are using before rendering the card, and they don't want to dynamically size their card because then they have to refresh the layout on the super view of an adaptivecard view. #Closed |
|
|
||
| if (img.size.width > 0) { | ||
| heightToWidthRatio = img.size.height / img.size.width; | ||
| contentholdingview = (ACRContentHoldingUIView *)[rootView getImageView:mediaKey]; |
There was a problem hiding this comment.
(ACRContentHoldingUIView *)[ [](start = 29, length = 28)
Curious: Is there something like a safe casting in ObjectiveC? Do we have the right level of compiler flags enabled to catch potential issues?
There was a problem hiding this comment.
Objective-C is a superset of c, but there are no other ways of casting.
| heightToWidthRatio = img.size.height / img.size.width; | ||
| contentholdingview = (ACRContentHoldingUIView *)[rootView getImageView:mediaKey]; | ||
| if (contentholdingview) { | ||
| view = contentholdingview.subviews[0]; |
There was a problem hiding this comment.
ews[0]; [](start = 43, length = 7)
Is there ever a chance that the subviews array is empty so that accessing the first element will result in an error/av? Should we be checking for it upfront or adding an assert condition before trying to index into it?
There was a problem hiding this comment.
The ACRContentHoldingUIView is a wrapper view to hold a subview such as UIImageView. The content view is created when there is an image view to add to the image view map. before the image view is added to the map, the view is added to the content view.
| // process play icon image | ||
| NSString *piikey = [NSString stringWithCString:[acoConfig getHostConfig] -> GetMedia().playButton.c_str() encoding:[NSString defaultCStringEncoding]]; | ||
| UIImage *playIconImage = imageViewMap[piikey]; | ||
| NSString *playIconKey = makeKeyForImage(acoConfig, @"media-playicon-image", pieces); |
There was a problem hiding this comment.
media-playicon-image [](start = 57, length = 20)
Avoid hard coded literals in the code - where is this coming from? Also is this going to have potential localization impact? SHould this be out in its own const strings class for easy localizability potentially?
There was a problem hiding this comment.
there will be no localization impact. the strings are only used internally.
There was a problem hiding this comment.
In general bad practice to hard code literals - whether they are to be localized or not.
In reply to: 509830950 [](ancestors = 509830950)
|
Is this going to cause our renderers to be inconsistent at this point? In reply to: 713932386 [](ancestors = 713932386) |
|
🎉 Handy links: |
* Added placeholder suport * Updated per CR comments
Related Issue
Fixed #4665 and Fixed #4682
Description
How Verified
How you verified the fix, including one or all of the following:
Microsoft Reviewers: Open in CodeFlow