Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Add task module samples #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add task module samples #65

wants to merge 1 commit into from

Conversation

nehasahu1505
Copy link

No description provided.

@msftclas
Copy link

msftclas commented Feb 11, 2019

CLA assistant check
All CLA requirements met.

@nehasahu1505 nehasahu1505 requested a review from aosolis February 11, 2019 19:45
@aosolis aosolis changed the title Added support for taskmodule Add task module samples Feb 13, 2019

switch (invokeType) {
case "task/fetch":
let taskModule = payload.data.taskModule.toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add comments here (in each "case" block") explaining conceptually what these invokes mean and how we respond to them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also mention which properties are standard from the framework, and which ones you added. For example, "taskModule" is something that you added in the "data" field of the adaptive card action. I'd like this to be clear because it's hard, when reading a sample, to separate what comes from the framework and what the developer added herself.

@@ -0,0 +1,111 @@
import * as builder from "botbuilder";
Copy link
Contributor

Choose a reason for hiding this comment

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

Name this file TaskModuleAdaptiveCardDialog.ts, and move it to the teams folder, not basic.

horizontalAlignment: "left",
wrap: false,
maxLines: 0,
speak: "<s>Adaptive card!</s>",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the speak elements, as it might imply that Teams supports speech.

actions: [
{
type: "Action.Submit",
title: "Invoke Task Module(Adaptive Card) -single step",
Copy link
Contributor

Choose a reason for hiding this comment

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

Be consistent with capitalization, here we have "single step", below we capitalize the first character.

@@ -72,6 +73,11 @@ export class DefaultTab {
document.getElementById('contextOutput').innerHTML = JSON.stringify(context);
});
}

function openTaskModule(){

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove blank line

taskInfo.height = 510;
taskInfo.width = 510;
let submitHandler = (err, result) => {
document.getElementById('taskModuleResult').innerHTML="you enterd the command: " + result.commandToBot;
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: enterd

taskInfo.height = 510;
taskInfo.width = 510;
let submitHandler = (err, result) => {
document.getElementById('taskModuleResult').innerHTML="you enterd the command: " + result.commandToBot;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, don't use the innerHTML property, as that can make the page susceptible to XSS. Use JQuery's text().

};
}

document.addEventListener("DOMContentLoaded", function(){
Copy link
Contributor

Choose a reason for hiding this comment

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

All this can be in the ready() handler (line 60).

public static getRequestHandler(): express.RequestHandler {
return async function (req: any, res: any, next: any): Promise<void> {
try{
let htmlPage = `<!DOCTYPE html> <head>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to use a handlebars template instead. It's easier to read.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants