-
Notifications
You must be signed in to change notification settings - Fork 378
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
feat: Custom Action Sample #2950
Conversation
48341cf
to
a41c84d
Compare
@@ -0,0 +1,100 @@ | |||
{ |
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 thought since we modified the built-in CSharp template (code + schema), we don't actually need this sample, every sample can be used as customized action sample.
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.
see #2824 (comment)
@@ -0,0 +1,8901 @@ | |||
{ |
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.
this should be an update to the default sdk.schema, instead of creating a new one.
@@ -0,0 +1,25 @@ | |||
{ |
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.
This is unnecessary, and doing the schema here and doing the code in another place actually make the CSharp Bot template not "self-contained".
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.
See above.
I expect the changes to be
- Only happened inside BotProject/CSharp/Template
- add a new MultiplyDialog.schema + MultiplyDialog.cs (self-contained change)
- update the sdk.schema to include this MultiplyDialog.schema (using dialog:merge)
and that's it, if user run ejection, it would just work, make sense?
If we want to keep the custom schema "custom", we need keep two versions of schema in composer:
which means, every time we update the sdk package+schema, we need to keep in mind to do twice of the schema merge for the above two, which doesn't look good for me. So my hacky way is to put custom schema in the sample folder and when ejection you can customize you sdk.schema by rerun the dialog:merge. I think both way are not perfect, how about we just explain how to use it in documents. I am not sure providing a single click experience on custom schema is a good direction which will make user confused. ("I don't need to do anything to enable custom schema"?) @srinaath is working on a readme to explain how to enable this. we can decide whether that is enough for user. if adding this sample just introduces more confusion, let's close this. |
I prefer to let user know how to merge the sdk.schema instead of just give them the customized sdk.schema as if user really use this feature later, he need to know how to do the schema merge. For custom schema, there are 2 steps.
|
@luhan2017 I used this PR to actually build my README file for adding custom actions. I think there is a lot of value in including this sample and I'm mentioning about this sample in the README too. Granted the PR does a little magic of showing the merged schema directly. But the README coupled with this sample should give a user a good idea to add custom actions. @hibrenda I will be making a PR today to close #2896 that includes the steps you have mentioned above. We can update that PR if the README is still confusing for someone starting up with Composer |
@@ -168,6 +169,9 @@ public void ConfigureServices(IServiceCollection services) | |||
var resourceExplorer = new ResourceExplorer().AddFolder(botDir); | |||
var rootDialog = GetRootDialog(botDir); | |||
|
|||
// register customized types, please note this is only needed for customized actions. | |||
resourceExplorer.RegisterType<MultiplyDialog>("MultiplyDialog"); |
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.
@luhan2017 so currently the default runtime that is ejected out is going to include this registration for all samples correct? Do you think if we could include this registration only if they eject out CustomActions sample?
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.
yes we should include it for everyone so they have a clear example of how it works.
@luhan2017 @benbrown should this be added to the functions runtime as well? If yes, we could place the dialog in the core project (/runtime/core) and then jsut reference it from the web app startup.cs and from the functions startup.cs. |
IMO - yes! |
To who evolved in the PR, @benbrown @benbrown @hibrenda @cwhitten @srinaath I have removed an action sample and added a custom action project in the runtime folder. which user need to refer to it explicitly to enable it. See the details at the beginning. @carlosscastro , if user want to enable it for azure function, he/she will need to add a project reference for azure function and add CustomActionComponentRegistration in Azurefunction/startup.cs. which I didn't explicitly add right now. |
Awesome @luhan2017 .. Adding my README commits to this PR too. Once added I think we can request a rereview and merge it |
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Readme updated Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
README updated and update.sh script to run bfdialog tool on Mac/Win added. PR ready for rereview |
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
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.
Now looks good to me
* Custom Action Sample * Update resultProperty type * fix the displayed title of customized action (microsoft#2951) * Update the Custom Action to a seperated solution * update * azure readme Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Readme updated Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> * Appednignmultiplydialog.schema Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> * Removed whitespace Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> * Fixed typo Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> * More readme updates Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Co-authored-by: zeye <2295905420@qq.com> Co-authored-by: Srinaath Ravichandran <srravich@microsoft.com> Co-authored-by: Srinaath Ravichandran <srinaath27@gmail.com> Co-authored-by: Andy Brown <asbrown002@gmail.com>
Description
In this PR, I added a CustomAction solution in the C# runtime folder which includes the custom code and schema.
Here are the steps to enable CustomAction:
Task Item
Create your bot based on custom actions and try out in emulator.
fixes #2824
fixes #2896
Screenshots