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

feat: <ExternalLink /> component enhancements #20903

Merged
merged 13 commits into from
Apr 7, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 9 additions & 10 deletions packages/app/src/runs/modals/CreateCloudOrgModal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,15 @@
<p class=" mb-16px text-gray-700">
{{ t('runs.connect.modal.createOrg.description') }}
</p>
<div @click="startWaitingOrgToBeCreated()">
<ExternalLink
class="border rounded mx-auto outline-none py-11px px-16px border-indigo-500 bg-indigo-500 text-white inline-block hocus-default max-h-60px"
:href="createOrgUrl"
:include-graphql-port="true"
>
<i-cy-office-building_x16 class="inline-block icon-dark-white" />
{{ t('runs.connect.modal.createOrg.button') }}
</ExternalLink>
</div>
<ExternalLink
class="border rounded mx-auto outline-none py-11px px-16px border-indigo-500 bg-indigo-500 text-white inline-block hocus-default max-h-60px"
:href="createOrgUrl"
:include-graphql-port="true"
@click="startWaitingOrgToBeCreated()"
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool - this is the click that made me suspect we needed to emit the event, I assumed the extra div I saw here was a workaround for this component swallowing the click event, looks like it wasn't but still better to have this here and let this part of the UI be keyboard-accessible.

>
<i-cy-office-building_x16 class="inline-block icon-dark-white" />
{{ t('runs.connect.modal.createOrg.button') }}
</ExternalLink>
</div>
<template #footer>
<div class="flex gap-16px">
Expand Down
1 change: 1 addition & 0 deletions packages/frontend-shared/src/components/BaseLink.vue
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<a
:href="props.href"
:class="classes"
@keypress.space.enter.prevent="open"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like it's needed, may have been left in PR accidentally? I took it out and everything was fine.

><slot /></a>
</template>

Expand Down
113 changes: 113 additions & 0 deletions packages/frontend-shared/src/gql-components/ExternalLink.cy.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
import ExternalLink from './ExternalLink.vue'
import { ExternalLink_OpenExternalDocument } from '../generated/graphql'

describe('<ExternalLink />', () => {
beforeEach(() => {
const onClickSpy = cy.spy().as('onClickSpy')

cy.mount(() => (
<ExternalLink
href='https://on.cypress.io/ci'
// @ts-ignore - vue @click isn't represented in JSX
onClick={onClickSpy}
>
Click me!
</ExternalLink>
))
})

it('opens external link on click', () => {
const urlStub = cy.stub()

cy.stubMutationResolver(ExternalLink_OpenExternalDocument, (defineResult, { url }) => {
urlStub(url)

return defineResult({
openExternal: true,
})
})

cy.get('[data-cy="external"]')
.click()

cy.wrap(urlStub).should('have.been.calledWith', 'https://on.cypress.io/ci')
cy.get('@onClickSpy').should('have.been.calledOnce')
})

it('opens external link on click and triggers onClick function', () => {
const urlStub = cy.stub()

cy.stubMutationResolver(ExternalLink_OpenExternalDocument, (defineResult, { url }) => {
urlStub(url)

return defineResult({
openExternal: true,
})
})

cy.get('[data-cy="external"]')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cy.get('[data-cy="external"]')
cy.contains('a', 'Click me!')

A couple of reasons for this suggestion:

  • confirm accessibility of component in general (should be a link)
  • confirm the slot content you added in the beforeEach renders in the expected place

.click()

cy.wrap(urlStub).should('have.been.calledWith', 'https://on.cypress.io/ci')
cy.get('@onClickSpy').should('have.been.calledOnce')
})

it('opens external link on enter', () => {
const urlStub = cy.stub()

cy.stubMutationResolver(ExternalLink_OpenExternalDocument, (defineResult, { url }) => {
urlStub(url)

return defineResult({
openExternal: true,
})
})

cy.get('[data-cy="external"]')
.focus()
.realPress('Enter')

cy.wrap(urlStub).should('have.been.calledWith', 'https://on.cypress.io/ci')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to confirm the onClickSpy was called here?

Suggested change
cy.wrap(urlStub).should('have.been.calledWith', 'https://on.cypress.io/ci')
cy.wrap(urlStub).should('have.been.calledWith', 'https://on.cypress.io/ci')
cy.get('@onClickSpy').should('have.been.calledOnce')

})

it('opens external link on click and enter', () => {
const urlStub = cy.stub()

cy.stubMutationResolver(ExternalLink_OpenExternalDocument, (defineResult, { url }) => {
urlStub(url)

return defineResult({
openExternal: true,
})
})

cy.get('[data-cy="external"]')
.click()

cy.get('[data-cy="external"]')
.focus()
.realPress('Enter')
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note, the need to use realPress here is a sign that we should look into doing something like #19726 for links as well. Feel like cy.type('{enter}') should work fine here, but it doesn't. I'll look into that separately and probably open an issue.

Also I think we can shorten this up a little if we want:

Suggested change
cy.get('[data-cy="external"]')
.click()
cy.get('[data-cy="external"]')
.focus()
.realPress('Enter')
cy.get('[data-cy="external"]')
.click()
.realPress('Enter')


cy.wrap(urlStub).should('have.been.calledTwice')
cy.get('@onClickSpy').should('have.been.calledTwice')
})

it('do not open external link on space bar trigger', () => {
const urlStub = cy.stub()

cy.stubMutationResolver(ExternalLink_OpenExternalDocument, (defineResult, { url }) => {
urlStub(url)

return defineResult({
openExternal: true,
})
})

cy.get('[data-cy="external"]')
.focus()
.realPress('Space')

cy.wrap(urlStub).should('not.be.called')
cy.get('@onClickSpy').should('not.be.called')
})
})
3 changes: 1 addition & 2 deletions packages/frontend-shared/src/gql-components/ExternalLink.vue
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
data-cy="external"
:href="props.href"
:use-default-hocus="props.useDefaultHocus"
role="link"
@click.prevent="open"
@keypress.space.enter.prevent="open"
@keypress.enter.prevent="open"
><slot /></BaseLink>
</template>

Expand Down