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

Intents and slots prompts confirmation #366

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fabien88
Copy link

Intent and slots confirmation with samples prompts like seen in #358

@fabien88 fabien88 force-pushed the intentsAndSlotsPrompts branch 5 times, most recently from 5efead2 to 0126697 Compare August 24, 2018 16:03
@matt-kruse
Copy link
Collaborator

This is a great improvement. I'm trying to add a Dialog to a skill right now and finding that I need the exact changes in this PR. This should be merged along with some other changes and a release made, otherwise this framework is going to fall behind the quickly-evolving API. @dblock any thoughts?

@Timvdv
Copy link

Timvdv commented Oct 4, 2018

This is a great feature! Hope it going to be merged soon.

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe @lazerwalker is a better code reviewer for this?

The build has failed for something that looks legit.

CHANGELOG.md Outdated
@@ -5,7 +5,8 @@
* [#344](https://github.com/alexa-js/alexa-app/pull/344): Add coveralls as a check for pull requests - [@kobim](https://github.com/kobim).
* [#352](https://github.com/alexa-js/alexa-app/pull/352): Allow utterance expansion in custom slot type synonyms - [@daanzu](https://github.com/daanzu).
* [#361](https://github.com/alexa-js/alexa-app/pull/361): Fix object reference for dialogState in in doc - [@Sephtenen](https://github.com/Sephtenen).
* Your contribution here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put this back :)

@fabien88 fabien88 force-pushed the intentsAndSlotsPrompts branch from 0126697 to 39abf5e Compare October 4, 2018 20:28
Copy link
Collaborator

@lazerwalker lazerwalker left a comment

Choose a reason for hiding this comment

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

I'm not particularly familiar with the elicitation/confirmation APIs, but this generally seems sensible to me?

intent,
key;

var creatPrompt = function(promptId, variations) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be named createPrompt, yeah?

var creatPrompt = function(promptId, variations) {
return {
id: promptId,
variations: variations.map(prompt => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this should check if the user's passed in an already-constructed response object, and if so just use that instead of string-concatenating SSML for them?

I'm not sure if I'm being silly, and maybe there's no reason anyone would want to do that in practice, but seeing SSML being manually constructed like this usually gives me pause.

@lazerwalker
Copy link
Collaborator

FYI, the CI failures (at least the ones on the latest node) are coming in via dtslint, the linter for our TypeScript type definitions. You need to add the new properties you've added to the appropriate places in the type definitions.

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.

5 participants