-
Notifications
You must be signed in to change notification settings - Fork 311
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
Add ability for Dev Home to display settings cards from an adaptive card #2488
Conversation
|
||
namespace DevHome.Common.Contracts; | ||
|
||
public interface IDevHomeActionRender : IAdaptiveElementRenderer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACs have IAdaptiveElementRenderer and IAdaptiveActionRenderer. This seems to be an interface for elements, so it would be called IDevHomeElementRenderer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this one is suppose to just be the custom renderer for an ActionSet but I can update the name
var parseResult = AdaptiveCard.FromJsonString(adaptiveCardString); | ||
AdaptiveCardParseResult parseResult; | ||
|
||
if (_actionParserRegistration != null && _elementParserRegistration != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe simpler: Have one constructor where the two *ParserRegistrations are optional, and do like
_elementParserRegistration = elementParserRegistration != null ? elementParserRegistration : new AdaptiveElementParserRegistration();
Then you don't need the if/else here or in ExtensionAdaptiveCardPanel.cs, I think. Also if in the future there was a case where we had only a custom action parser or only element parser, it would handle that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try that out
…om holding onto items view control internally
I'm not sure there's a way around this (I'd hope so, but I'm a "git rebase" person not a "git merge" person), but this PR contains a lot of the changes that are already merged into main, making it difficult to review right now. |
@krschau whoops, I merged main into this branch but forgot to merge main into the feature branch. After merging main into the feature branch, only my changes look to be appearing now. Sorry about that |
/// <returns> | ||
/// A boolean indicating whether validation for the card passed or failed. | ||
/// </returns> | ||
public bool TryValidateAndInitiateAction(string buttonId, AdaptiveInputs userInputs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/microsoft/devhome/pull/2493/files#diff-9aa0e204d624670b424e4a4de48b0aa416de4f630d62b806e408f15274943c18R293 looks like userInputs
will be null
if the ViewModel is null
.
crash if null(?)
common/DevHomeAdaptiveCards/CardInterfaces/IDevHomeSettingsCardNonSubmitAction.cs
Show resolved
Hide resolved
Co-authored-by: Kristen Schau <47155823+krschau@users.noreply.github.com>
return true; | ||
} | ||
|
||
if (_itemsView.SelectedItem == null || _itemsView.CurrentItemIndex < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding guidelines: "Use parentheses to make clauses in an expression apparent".
This is in Windows C++ guidelines and in C# Common code conventions.
I feel like a couple of screenshots with highlighted IDevHomeSettingsCardNonSubmitAction button and DevHomeSettingsCard would be more informative than the attached video where I don't really know what to look at. |
…hub.com/microsoft/devhome into user/bbonaby-add-adaptive-card-updates2
Summary of the pull request
This PR is 1 of 4 other PRs that I'll be submitting to allow creation. (I'm putting them in a feature branch first)
As part of the work to allow extensions to create environments within Dev Home, we wanted to give them the ability to use adaptive cards as the UI. This is because we won't know all the different capabilities/controls an extension wants to show in their UI, so using adaptive cards for this was the natural choice. However adaptive cards can be constraining in what it supports out of the box. With that said they give us the ability to create custom renders and custom types. To do this we also need to create a custom parser for the custom types. I also added a custom type to launch a dialog an adaptive card. This PR also allows us to invoke an adaptive cards submit button from within Dev Home.
The settings adaptive settings card will show up in the second page of the creation flow (if an extension chooses to use it)
The UI for the video below will be in a separate PR but this shows me getting the cards from the Hyper-V Extension. Alot of this code is just Parsing Json.
CreateEnvironmentFlowUpToReviewPage.mp4
Note: I'm ok with any changes that need to be reworked, as I'd like this to be a general solution we can use in Dev Home. We do plan on circling back in the future to see if we can add a listview type support to the official adaptive cards codebase. However, that is future work.
References and relevant issues
Detailed description of the pull request / Additional comments
Here is an actual example of the usage. Its what we're using in the Hyper-V extension when sending our adaptive card to Dev Home.
Validation steps performed
PR checklist