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

a11y: reapply tooltip additions #3362

Merged
merged 22 commits into from
Jun 18, 2020
Merged

a11y: reapply tooltip additions #3362

merged 22 commits into from
Jun 18, 2020

Conversation

beyackle
Copy link
Contributor

@beyackle beyackle commented Jun 9, 2020

Description

This is a reapplication of the changes from @corinagum 's work on #2691, after that old PR and the master branch diverged enough that trying to rebase just leading to a larger and larger mess over time.

Task Item

refs #2006

Screenshots

See screenshots in #2691

@coveralls
Copy link

coveralls commented Jun 9, 2020

Coverage Status

Coverage increased (+0.04%) to 48.094% when pulling 59421f5 on beyackle/2006TooltipRedo into c203a7c on master.

@@ -8,7 +8,7 @@ context('LU Page', () => {
});

it('can open language understanding page', () => {
cy.findByTestId('LeftNav-CommandBarButtonUser Input').click();
cy.findByTestId('LeftNav-CommandBarButtonUser Input').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.

Let's make this work without using force.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure we can. Without force, the e2e tests were failing because, while Cypress could see the element it wanted to click, it complained that the element was being covered by something, and some investigation showed that that something else was the tooltip's active region. I could see if making Cypress click on the tooltip region works, but that doesn't seem like a natural way to test the behavior we actually want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On further inspection, the e2e tests are still failing even with force, so I'll figure out another way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for looking at this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like those failures were transient. They were definitely failing without force, though, so I think we still might need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should figure out why we need it. To me, this lowers my confidence in these tests because it could lead to false positives.

@@ -59,7 +59,7 @@ context('Notification Page', () => {
cy.findByTestId('FieldErrorMessage').should('exist');
});

cy.findByTestId('LeftNav-CommandBarButtonNotifications').click();
cy.findByTestId('LeftNav-CommandBarButtonNotifications').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.

Ditto

Composer/packages/client/src/App.tsx Outdated Show resolved Hide resolved
@a-b-r-o-w-n a-b-r-o-w-n self-assigned this Jun 16, 2020
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.

Just two minor things!

@@ -41,7 +41,7 @@ context('Notification Page', () => {
cy.findAllByText('__TestToDoBotWithLuisSample').should('exist');
});

it('can show dialog expression error ', () => {
it.only('can show dialog expression error ', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

whoops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Yeah, I'll fix that.

Composer/cypress/integration/LUPage.spec.ts Outdated Show resolved Hide resolved
@cwhitten cwhitten merged commit 9281677 into master Jun 18, 2020
@cwhitten cwhitten deleted the beyackle/2006TooltipRedo branch June 18, 2020 17:45
@cwhitten cwhitten mentioned this pull request Jul 8, 2020
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
* reapply tooltip additions

* maybe fix e2e tests

* merge and resolve conflicts

* Update OpenObjectField.tsx

* remove force:true

* fix logic on nav tooltips

* Update LUPage.spec.ts

* re-remove plugins directory (moved in another commit)

* add parentsUntil to tests

* fix tests

* fix tests

* defocus tooltips during tests
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.

4 participants