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: setup launchpad lifecycle #18734

Merged
merged 13 commits into from
Nov 4, 2021

Conversation

ImCesar
Copy link
Contributor

@ImCesar ImCesar commented Nov 2, 2021

Set up the launchpad life cycle if the user has been through it before. If it's users first time they will select the type of testing and browser to run. If they have chosen those before then it will skip the launchpad and go straight to the app.

Also adds a button to the settings page that allows the user to reconfigure their preferences.

Demo

Launchpad.Workflow.mov

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 2, 2021

Thanks for taking the time to open a PR!

@ImCesar ImCesar requested review from lmiller1990 and a team and removed request for lmiller1990 November 2, 2021 00:08

const showLaunchpad = useMutation(App_ShowLaunchpadDocument)

window.addEventListener('beforeunload', () => {
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 tried doing this and some Vue lifecycle functions but they only worked sometimes. It seems like there is a race to for the network request to finish going through.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I am missing some context; when is this supposed to be triggered? Eg, when is the beforeunload event called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the user exits the app I want it to show the launchpad at the browser launch screen

@cypress
Copy link

cypress bot commented Nov 2, 2021



Test summary

8417 0 100 4Flakiness 3


Run details

Project cypress
Status Passed
Commit c6b143c
Started Nov 3, 2021 11:45 PM
Ended Nov 3, 2021 11:57 PM
Duration 12:12 💡
OS Linux Debian - 10.9
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/net_stubbing_spec.ts Flakiness
1 network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
2 network stubbing > waiting and aliasing > can timeout incrementally waiting on requests
e2e/redirects_spec.js Flakiness
1 redirection > meta > binds to the new page after a timeout

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

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Left some comments, might be good to do a pair code review? I think I'm not 100% clear on the desired behavior, I def need more context to give a more thorough review.


const showLaunchpad = useMutation(App_ShowLaunchpadDocument)

window.addEventListener('beforeunload', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I am missing some context; when is this supposed to be triggered? Eg, when is the beforeunload event called?

await this.actions.project.setActiveProject(this.config.launchArgs.projectRoot)

if (this.coreData.app.activeProject?.preferences) {
toAwait.push(this.actions.project.launchProjectWithoutLaunchpad())
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should name this something more generic, like launchElectron? Eg, we might one day not have a launchpad, but it's likely we will always have some kind of electron app running.

const { browserId, testingType } = preferences[this.ctx.activeProject.title] ?? {}

if (!browserId || !testingType) {
throw Error('Cannont launch project without stored browserId or testingType')
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here, Cannont. :)

throw Error('Cannont launch project without stored browserId or testingType')
}

const spec = this.makeSpec(testingType)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is hard requirement for now, but eventually we should not need to do this, I think... - maybe we can add a TODO to remove this when we refactor launchProject to not require a spec on launch.

Also, since makeSpec isn't used anywhere else, should we just do something like:

const spec = {
  name: '...'
  // ...
}

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 took this from the launchProject and forgot to add it back there. I can add this function there as well so when we refactor it to not require a spec we can clean it up in both spots.

@@ -1,7 +1,7 @@
<template>
<form
v-if="props.gql.browsers"
@submit.prevent="$emit('launch')"
@submit.prevent="emit('launch', props.gql.selectedBrowser.id)"
Copy link
Contributor

Choose a reason for hiding this comment

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

What changed that we need to now include the id? I don't see that change anywhere in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now when we launch we are storing their selected browser, now that I think about it I might be able to pull this off of the graphql context since we store the selected browser. I needed to refactor this anyways because id seems to come from graphql but not actually on our FoundBrowser type

@ImCesar ImCesar marked this pull request as ready for review November 3, 2021 17:28
@ImCesar ImCesar requested a review from a team as a code owner November 3, 2021 17:28
@ImCesar ImCesar requested review from jennifer-shehane and removed request for a team and jennifer-shehane November 3, 2021 17:28
@lmiller1990 lmiller1990 self-requested a review November 3, 2021 23:36
@lmiller1990 lmiller1990 merged commit 1158bcd into unified-desktop-gui Nov 4, 2021
@lmiller1990 lmiller1990 deleted the feat-use-launchpad-preferences branch November 4, 2021 00:14
tgriesser added a commit that referenced this pull request Nov 4, 2021
* unified-desktop-gui:
  chore: add percy to app and launchpad package (#18781)
  chore: update test
  refactor: move settings in app (#18729)
  feat: setup launchpad lifecycle (#18734)
tgriesser added a commit that referenced this pull request Nov 4, 2021
…ve-activeProject

* unified-desktop-gui: (57 commits)
  chore: Add e2e tests for global mode (#18719)
  chore: add percy to app and launchpad package (#18781)
  chore: update test
  refactor: move settings in app (#18729)
  feat: setup launchpad lifecycle (#18734)
  feat(app): decouple event manager from driver (#18695)
  chore: Force single resolution for core modules, infinite loop guard (#18764)
  fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to (#18441)
  chore: cleaning up the runner container pattern (#18741)
  feat: Use .config files (#18578)
  chore(app): basic style and example to stop scrollIntoView bug (#18736)
  chore: make `create` function on server.ts obsolete (#18615)
  feat: add codegen utility (#18708)
  docs: Add instructions to squash commits to develop in Contributing (#18728)
  fix(@cypress/react): throw if using Next.js swc-loader without nodeVersion=system (#18686)
  refactor: remove Ramda (#18723)
  fix: support using create-cypress-tests as part of build process (#18714)
  chore: Increase paralleled machines for desktop-gui tests (#18725)
  fix(app): do not cache graphql (#18716)
  chore: Update Chrome (stable) to 95.0.4638.69 (#18696)
  ...
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.

None yet

2 participants