-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adaptive Cards #471
Adaptive Cards #471
Conversation
@danmarshall What is the current timeline for Adaptive Cards full support in Web Chat? |
A few more days of core development to get code complete, another few days to get documentation and tests in place. |
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.
Here's my first pass. I reserve the right to make more passes. :-)
src/AdaptiveCardContainer.tsx
Outdated
} | ||
|
||
AdaptiveCards.AdaptiveCard.onExecuteAction = (action: AdaptiveCards.ExternalAction) => { | ||
|
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.
Cleanup the extra carriage returns through all of the files
src/AdaptiveCardContainer.tsx
Outdated
} | ||
|
||
} else if (action instanceof AdaptiveCards.HttpAction) { | ||
/** |
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.
If we're not going to support HTTP Actions to start we should simply ignore the buttons, UI, etc.
src/AdaptiveCardContainer.tsx
Outdated
return content.slice(1, -1).replace(/\\"/g, '"').replace(/\\\\/g, '\\'); | ||
} | ||
|
||
public configFromJsonInCss() { |
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.
We should be able to remove this now as per our last conversation.
src/Attachment.tsx
Outdated
@@ -170,34 +169,44 @@ export const AttachmentView = (props: { | |||
case "application/vnd.microsoft.card.hero": | |||
if (!attachment.content) | |||
return null; | |||
|
|||
const heroCardBuilder = new CardBuilder.AdaptiveCardBuilder(); |
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.
We should create helper methods to create the various card types for readability.
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.
Or, maybe create derived classes from AdatpiveCardBuilder for the types? I'm not sure which would look better.
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 actually had this in a past revision. Made it less maintainable because we have this big switch statement here which consolidates all the types, and didn't want to have a structure like that in 2 places.
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 don't know that I'm convinced. :-) But I'm not going to make this a showstopper.
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.
Consider that this file used to have HTML for all cards.
* edits from feedback * moved Action.Http
Enable adaptive cards tests
* receipt card test Adding support for receipt card * Recept Card in version 0.5 Recept Card in version 0.5 * load adaptivecards hostconfig from json file, webpacked into bundle (#495) * remove Action.Http from cards * corrected alpha channel colors * add AdaptiveCards.md * rename to Bot Builder SDK * change SDK to Bot Builder SDK * fix typos * elsewhere * lowercase titles * added 'it' * separates to separate * handle undefined * imBack for string * Small language tweaks (#503) * Adaptivecards (#505) * edits from feedback * moved Action.Http
* Adaptive receipt (#2) * receipt card test Adding support for receipt card * Recept Card in version 0.5 Recept Card in version 0.5 * load adaptivecards hostconfig from json file, webpacked into bundle (#495) * remove Action.Http from cards * corrected alpha channel colors * add AdaptiveCards.md * rename to Bot Builder SDK * change SDK to Bot Builder SDK * fix typos * elsewhere * lowercase titles * added 'it' * separates to separate * handle undefined * imBack for string * Small language tweaks (#503) * Adaptivecards (#505) * edits from feedback * moved Action.Http * readme.md (#491) updated version of readme.md * edited test/README.md (#510) * edited test/README.md * Update README.md * error handling Adding SVG to represent the error occures in the JSON parsing or adaptive card missing properties. * Revert "error handling" This reverts commit 92a4d27. * error handling for ac 1. using this.forceUPdate() 2. svg error message * minior fixes * use state for error rendering * change forceUpdate() to setState() change forceUpdate() to setState() * merge * fix minor bug in ac tests * add style to error * latest AdaptiveCards lib * remove 'lighter' text * reformatted * explicit if statement * catch hyperlink clicks
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.
The core of the code looks just fine to me. There is some cleanup that needs to be done, though.
AdaptiveCards.md
Outdated
@@ -0,0 +1,91 @@ | |||
# Adaptive Cards in Microsoft Bot Framework WebChat | |||
|
|||
WebChat now supports [Adaptive Cards](http://adaptivecards.io/), which you can use to achieve a greater level of rich display and interactivity in your bot's UX. Adaptive Cards are a general-purpose technology, which may be used in a wide variety of applications, but you should be aware of some constraints when using them within the WebChat environment. |
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.
Flesh this out a touch more to address the fact that all channels will have limitations WRT Adaptive Cards, and not something that's unique to WebChat.
AdaptiveCards.md
Outdated
|
||
## Style customization | ||
|
||
|
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.
One too many carriage returns.
src/AdaptiveCardContainer.tsx
Outdated
|
||
function cardWithoutHttpActions(card: AdaptiveCardSchema.ICard) { | ||
if (!card.actions) return card; | ||
|
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.
Please cleanup the extra carriage returns in this file.
src/AdaptiveCardContainer.tsx
Outdated
if (!this.props.onClick) return; | ||
|
||
//do not allow form elements to trigger a parent click event | ||
switch ((e.target as HTMLElement).tagName) { |
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.
Will it always be capital letters?
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
src/AdaptiveCardContainer.tsx
Outdated
adaptiveCard.parse(cardWithoutHttpActions(this.props.card)); | ||
const errors = adaptiveCard.validate(); | ||
|
||
if (errors.length === 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.
Can errors
be 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.
it will always return an array
src/Attachment.tsx
Outdated
}); | ||
|
||
attachment.content.items && attachment.content.items.map((item, i) => { | ||
|
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.
Carriage return cleanup for this file, please
src/Attachment.tsx
Outdated
@@ -170,34 +169,44 @@ export const AttachmentView = (props: { | |||
case "application/vnd.microsoft.card.hero": | |||
if (!attachment.content) | |||
return null; | |||
|
|||
const heroCardBuilder = new CardBuilder.AdaptiveCardBuilder(); |
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 don't know that I'm convinced. :-) But I'm not going to make this a showstopper.
src/scss/botchat.scss
Outdated
margin-bottom: 20px; | ||
text-align: inherit; | ||
} | ||
|
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.
White space cleanup
test/mock_dl/index.ts
Outdated
}) | ||
if (queue) { | ||
if (queue.length > 0) { | ||
let msg = queue.shift(); |
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.
Could these be constants?
@@ -34,7 +35,16 @@ module.exports = { | |||
// All files with a '.ts' or '.tsx' extension will be handled by 'awesome-typescript-loader'. | |||
{ test: /\.tsx?$/, loader: "awesome-typescript-loader" }, | |||
// All output '.js' files will have any sourcemaps re-processed by 'source-map-loader'. | |||
{ enforce: "pre", test: /\.js$/, loader: "source-map-loader" } |
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.
Please cleanup the formatting here
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.
done
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.
done
It seems as if this was merged, but I am still getting this issue microsoft/AdaptiveCards#432 -- is there something that I should do? |
** Please do not merge this PR - this is for review only, until the checklist has been completed **
Must haves:
Stretch goals (if not checked these should become issues after the PR):
Add a watcher on SCSS to call ac-scss2jsonPagination of buttons when there is more than 5