feat: <ExternalLink /> component enhancements #20903
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| @@ -3,6 +3,7 @@ | |||
| <a | |||
| :href="props.href" | |||
| :class="classes" | |||
| @keypress.space.enter.prevent="open" | |||
There was a problem hiding this comment.
This doesn't look like it's needed, may have been left in PR accidentally? I took it out and everything was fine.
| .focus() | ||
| .realPress('Enter') | ||
|
|
||
| cy.wrap(urlStub).should('have.been.calledWith', 'https://on.cypress.io/ci') |
There was a problem hiding this comment.
Do we want to confirm the onClickSpy was called here?
| 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') |
| cy.get('[data-cy="external"]') | ||
| .click() | ||
|
|
||
| cy.get('[data-cy="external"]') | ||
| .focus() | ||
| .realPress('Enter') |
There was a problem hiding this comment.
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:
| cy.get('[data-cy="external"]') | |
| .click() | |
| cy.get('[data-cy="external"]') | |
| .focus() | |
| .realPress('Enter') | |
| cy.get('[data-cy="external"]') | |
| .click() | |
| .realPress('Enter') |
| }) | ||
| }) | ||
|
|
||
| cy.get('[data-cy="external"]') |
There was a problem hiding this comment.
| 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
| 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()" |
There was a problem hiding this comment.
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.
|
@estrada9166 why do we want to change the behavior of the ExternalLink component to not behave like a link and open when the space key is pressed? |
Regular links only open with the enter key, the space behavior we added originally here is not standard - so it would be unexpected behavior if a user whose keyboard is focused on a link hits spacebar to do its default thing (usually scroll the page) and we open the link. |
| 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.contains('a', 'Click me!') | ||
| .click() | ||
|
|
||
| cy.wrap(urlStub).should('have.been.calledWith', 'https://on.cypress.io/ci') | ||
| cy.get('@onClickSpy').should('have.been.calledOnce') | ||
| }) |
There was a problem hiding this comment.
This guy is 1:1 with 'opens external link on click'
User facing changelog
Additional details
How has the user experience changed?
Prevent opening external links whit the spacebar
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?