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

Added new HeroCard prompt style for ChoicePrompt as per #1170 #1339

Merged
merged 4 commits into from
Jan 30, 2019

Conversation

garypretty
Copy link
Contributor

@garypretty garypretty commented Jan 29, 2019

Fixes: #1170 #1335 #1173

This is to provide support (other than plain text) for channels that do not support suggested actions, such as Teams. Test added.

… This is to provide support (other than plain text) for channels that do not support suggested actions, such as Teams.
@v-kydela
Copy link
Contributor

@garypretty - Looks like a great PR! I looked at the code and saw that ChoiceFactory.ForChannel still needs to be updated to use your new hero card option. Should that be in a different pull request?

@garypretty
Copy link
Contributor Author

@v-kydela I am having a discussion over on the Virtual Assistant / Enterprise Template repo at the moment about this and I think it probably makes sense to include it in this PR. I will try to get an update in shortly. Would be great to get this in as I think it is also blocking some work over there. cc. @darrenj

…support SuggestedActions but do have a message feed (right now this appears to just be Teams).
@garypretty
Copy link
Contributor Author

garypretty commented Jan 30, 2019

@v-kydela I have updated this PR with a change to ChoiceFactory.ForChannel to catch the Teams / Cortana scenario (supports cards but not suggested actions) and to use HeroCard in this instance.

@darrenj @lauren-mills Hopefully this will help resolve your issue (https://github.com/Microsoft/AI/issues/654) too.

@garypretty
Copy link
Contributor Author

@cleemullins This is currently tagged up for 4.4 in #1170 but it seems getting this in sooner will help unblock some customers and blocking issues over on the VA / ent template side too.

@darrenj
Copy link

darrenj commented Jan 30, 2019

Agreed here is an example blocking work with a key customer and we've had a lot of feedback about the fidelity of our teams support especially as we push into enterprise scenarios.

@cleemullins
Copy link
Contributor

I'm fine with this change, although I have high-level checks to make first:

  1. @Stevenic needs to chime in. He's our Card Expert. :)
  2. We need to track the JS work to keep the two in parity. An issue in the JS repo, linked here, would be enough.

@cleemullins
Copy link
Contributor

We've got standup scheduled at 1pm today and I'll get closure from @Stevenic then.

@garypretty If you would like to join us via Teams, drop me an email.

@garypretty
Copy link
Contributor Author

garypretty commented Jan 30, 2019

@cleemullins I have raised the issue on the JS side as requested. microsoft/botbuilder-js#736. Also dropped you a note re. the standup.

Copy link
Contributor

@Stevenic Stevenic left a comment

Choose a reason for hiding this comment

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

:shipit:

@cleemullins
Copy link
Contributor

JS Tracking bug here: microsoft/botbuilder-js#737

@Stevenic
Copy link
Contributor

I agree that this is a semi-major issue with a relatively simple fix. Thanks for doing the C# side @garypretty

@v-kydela
Copy link
Contributor

If I may nitpick a bit, I noticed that the introduction of hero cards into ChoiceFactory.ForChannel has changed a convention we were using. The list styles appeared in the if/else blocks in order of preference, and that order was suggested actions, inline, list. With the introduction of hero cards, the order of preference is now suggested actions, hero card, inline, list, and yet the order they appear in is hero card, suggested actions, inline, list. When reading the code now, it is unclear what the order of preference is. However, that can wait for another PR since that's just about conventions rather than logic.

I actually think it would be more logical to order them from least preferable to most, and that would be list, inline, hero card, suggested actions. That way the first line could just be if (longTitles) and then longTitles wouldn't need to be mentioned after that.

@garypretty garypretty deleted the choice-card-style branch January 30, 2019 21:36
@garypretty
Copy link
Contributor Author

@v-kydela good point - nothing wrong with nitpicking 👍

ShYuPe pushed a commit to ShYuPe/botbuilder-dotnet that referenced this pull request Aug 25, 2020
* Add end_on_invalid_message and fix timeout issue

* fixing pylint

Co-authored-by: Axel Suarez <axsuarez@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

5 participants