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

Updated handling missing text property of TextBlock #4765

Merged
merged 38 commits into from
Sep 29, 2020

Conversation

jwoo-msft
Copy link
Member

@jwoo-msft jwoo-msft commented Sep 11, 2020

Related Issue

reference #4478
reference #4801
reference #4305

Description

Updated Android / UWP to respect the new parsing / rendering scheme.
Renderers don't fail to render the entire card. Renders make rendering decisions per element.
Renderers will try to render card after they encounter a TextBlock with no text.

How Verified

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

  1. A new unit test is added.
  2. Regression tests are run on 1.0 TextBlocks cards
    will aid in code reviews or corresponding fixes on other platforms for eg.***
Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the no-recent-activity label Sep 16, 2020
@ghost
Copy link

ghost commented Sep 16, 2020

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.

@ghost ghost removed the no-recent-activity label Sep 21, 2020
@ghost
Copy link

ghost commented Sep 21, 2020

Hi @jwoo-msft; Thanks for taking action on your previously stale pull request. Resetting staleness.

{
SetText(ParseUtil::GetString(json, AdaptiveCardSchemaKey::Text, true));
SetText(ParseUtil::GetString(json, AdaptiveCardSchemaKey::Text, false));
Copy link
Contributor

@RebeccaAnne RebeccaAnne Sep 24, 2020

Choose a reason for hiding this comment

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

GetString [](start = 23, length = 9)

We actually want this behavior for all required properties going forward now, right (#4565)? Rather than adding this code here specifically in TextBlock, do we want to update GetString to not throw and to instead add a warning when a required property is missing? That probably requires some follow up in the renderers to make sure that we don't crash on missing properties. But otherwise we'll end up with all "required" properties having this block of code and never passing true to the ParseUtil functions.
#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.

As discussed, I've created a new issue to track to update all of the required string parsing as part of #4565. #4834 is the one track this particular issue.


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

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

@RebeccaAnne RebeccaAnne Sep 24, 2020

Choose a reason for hiding this comment

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

Can we give this file a more descriptive name? #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.

I will remove this card, and merge this card with one in the FactSet into a new card on a new PR.


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

almedina-ms and others added 4 commits September 28, 2020 12:44
* perf investigation

* investigation changes

* Updated Default Value Handling

* Updated app to have explicit render button

* reverted button changes

* addressed review comments
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 866fba3 into main Sep 29, 2020
@jwoo-msft jwoo-msft deleted the jwoo/TextBlockUpdate branch September 29, 2020 05:26
rankush pushed a commit to rankush/AdaptiveCards that referenced this pull request May 8, 2024
* work in progress

* updated ChoiceSetInput.cpp

* changed the code such that missing text doesn't throw exception

* updated TextBlock Test

* uwp changes

* android changes

* Updated Shared Model to include parse warnings

* [WPF] Change TextBlock to render only when text is not empty (microsoft#4799)

* Change default value and change rendering behavior

* Add skip to failing test

* perf investigation

* investigation changes

* Updated Default Value Handling

* Updated app to have explicit render button

* changed the code such that missing text doesn't throw exception

* updated TextBlock Test

* uwp changes

* android changes

* Updated Shared Model to include parse warnings

* more changes

* Android Changes

* work in progress

* FactSet for dotnet

* ios change

* Remove warnings for elements that failed to render and don't perform fallback on them

* addressed CR review comments

* Updated dotnet Additional Properties Handling (microsoft#4800)

* perf investigation

* investigation changes

* Updated Default Value Handling

* Updated app to have explicit render button

* reverted button changes

* addressed review comments

* removed FactSet.Test.json to include comprehensive cards from a new PR

* Removed TextBlock.Test.json for udpated card which will be added in
separate PR

* updated idl

* updated factset test

Co-authored-by: shalinijoshi19 <shalinij@microsoft.com>
Co-authored-by: almedina-ms <35784165+almedina-ms@users.noreply.github.com>
Co-authored-by: Alberto Medina Gutierrez <almedina@microsoft.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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants