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.

Sign up for GitHub

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

Jump to bottom

feat: Create bot flow #2521

Merged
merged 10 commits into from
Apr 8, 2020
Merged

feat: Create bot flow #2521

merged 10 commits into from
Apr 8, 2020

Conversation

liweitian
Copy link
Contributor

@liweitian liweitian commented Apr 6, 2020 •
edited
Loading

Description

Applied new design for create bot option 3.

Task Item

Closes #1728

Screenshots

aaaaa

@cwhitten
Copy link
Member

cwhitten commented Apr 6, 2020

@liweitian can you please add some tests and also address the currently failing Unit and E2E tests? Thank you.

cwhitten
cwhitten requested changes Apr 6, 2020
View reviewed changes
Copy link
Member

@cwhitten cwhitten left a comment

Choose a reason for hiding this comment

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

see earlier comment

@github-actions
Copy link

github-actions bot commented Apr 7, 2020

Coverage Status

Coverage increased (+0.01%) to 40.473% when pulling 1dc508c1f634a226990c26d9cca7c3914b75e50f on liweitian:createBotFlow into 9003876 on microsoft:master.

cwhitten
cwhitten requested changes Apr 8, 2020
View reviewed changes
Copy link
Member

@cwhitten cwhitten left a comment

Choose a reason for hiding this comment

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

I don't think we need to plumb through the createdTime values.

Composer/packages/client/src/CreationFlow/CreateOptions/index.js Outdated Show resolved Hide resolved
Composer/packages/client/src/CreationFlow/CreateOptions/index.js Outdated Show resolved Hide resolved
Composer/packages/client/src/CreationFlow/CreateOptions/index.js Outdated Show resolved Hide resolved
a-b-r-o-w-n
a-b-r-o-w-n approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n 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 going to do a function pass before merging.

Composer/cypress/integration/CreateNewBot.spec.ts
@@ -21,7 +21,7 @@ context('Creating a new bot', () => {

it('can create a bot from the ToDo template', () => {
cy.findByTestId('Create from template').click();
cy.findByTestId('TodoSample').click();
cy.findByTestId('TodoSample').click({ force: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid this? Why is it necessary? Things like this take away from the trustworthiness of our tests.

Copy link
Contributor Author

@liweitian liweitian Apr 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

cypress-io/cypress#3450
cypress-io/cypress#871
See above issues. the click() method will trigger a weird scroll operation making our bot template invisible. Then we cannot click the template. Using the { force: true } can make it click the template before the scrolling operation. There is a merged PR on this issue which can let user choose whether to scroll or not. I tried it but our current cypress version does not support this. And I checked the official doc. It does not list that option. Perhaps it does not release yet?

Composer/cypress/support/commands.ts
@@ -9,7 +9,7 @@ Cypress.Commands.add('createBot', (bobotId: string, botName?: string) => {
cy.findByText('New').click();
});
cy.findByTestId('Create from template').click({ force: true });
cy.findByTestId(`${bobotId}`).click();
cy.findByTestId(`${bobotId}`).click({ force: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment.

Composer/packages/client/src/CreationFlow/CreateOptions/index.js
return (
<Fragment>
<ChoiceGroup
label={formatMessage('Choose how to create your bot')}
defaultSelectedKey="Create from scratch"
defaultSelectedKey="CreateFromScratch"
Copy link
Contributor

Choose a reason for hiding this comment

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

Its probably a good idea to make this and CreateFromTemplate a const value.

@cwhitten cwhitten merged commit 380fd7f into microsoft:master Apr 8, 2020
@cwhitten cwhitten mentioned this pull request Apr 8, 2020
Closed
@a-b-r-o-w-n a-b-r-o-w-n mentioned this pull request May 15, 2020
Merged
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
@liweitian @cwhitten
feat: Create bot flow (microsoft#2521)
5095f76 
* create new bot flow UI

* lint fix

* update css

* update css

* update css

* fix test case

* handle comments

* remove unused css

Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwhitten cwhitten cwhitten approved these changes

@a-b-r-o-w-n a-b-r-o-w-n a-b-r-o-w-n approved these changes

@boydc2014 boydc2014 Awaiting requested review from boydc2014 boydc2014 is a code owner

@corinagum corinagum Awaiting requested review from corinagum

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@liweitian @cwhitten @a-b-r-o-w-n