-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
fix: make launchpad link open default browser #20399
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ FlakinessThis 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 |
@@ -97,7 +98,7 @@ const props = defineProps<{ | |||
description?: string | |||
}>() | |||
|
|||
const openDocs = () => window.open('https://on.cypress.io/guides/configuration') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could just set the href
prop on the Button and not have to worry about using the right mutation here:
href?: string // will cause the button to render as link element with button styles |
Buttons with an href prop will use the external link mutation by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not be cool with router links...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what the internalLink
/to
props are for 😛 lots of props on Button apparently
internalLink?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... awwwww no! that's horrible.
I can do that... even if I am dying inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate that <Button>
takes a href
but this seems like the way to go. We could fix this eventually all in one shot. Do you plan to do this @elevatebart ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as Tyler! What do you think about that @elevatebart?
cy.findByRole('button', { name: 'Continue' }).click() | ||
cy.intercept('POST', 'mutation-ExternalLink_OpenExternal').as('OpenExternal') | ||
cy.findByText('Learn more.').click() | ||
cy.wait('@OpenExternal') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assert the correct arg was included in the request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: probably irrelevant if you make the change mentioned by @tbiethman.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in commit 15206c9
@@ -97,7 +98,7 @@ const props = defineProps<{ | |||
description?: string | |||
}>() | |||
|
|||
const openDocs = () => window.open('https://on.cypress.io/guides/configuration') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate that <Button>
takes a href
but this seems like the way to go. We could fix this eventually all in one shot. Do you plan to do this @elevatebart ?
@tbiethman @lmiller1990 Updated in the use of href in cad49a1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responded
Gonna merge since the request of Tyler is already done ✔️ |
* 10.0-release: fix: comment link to accurate docs (#20437) fix: scaffold commands file (#20398) fix(launchpad): support default export (#20383) feat(launchpad): support for Vue CLI 5 (#20413) fix: UNIFY-676 browsers should be configurable in setupNodeEvents (#20367) fix: make launchpad link open default browser (#20399) fix(icons): publish the files in the package fix: build icons in build-prod (#20411) test: migrate module-api to 10.0 chore: build this branch test: migrate module_api to system tests (#20265) chore: remove pkg/driver //@ts-nocheck final (#20169) chore: fix "cannot find module" in clone-repo-and-checkout-release-branch (#20293) chore: Update Chrome (beta) to 99.0.4844.45 (#20234) chore: fix CI cache state for darwin (#20339) Add TODO comments feedback chore: move tests to its own file.
User facing changelog
Instead of the link below (Learn more.) opening in a new electron window, it now opens in the default browser.