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(unify): allow prompts to auto-open after set time #19571

Merged
merged 21 commits into from
Jan 10, 2022

Conversation

marktnoonan
Copy link
Contributor

@marktnoonan marktnoonan commented Jan 5, 2022

This is a cleaned up version of #18743 with a couple of minor changes to adapt to the latest and greatest setup.

Additional details

This recreates the functionality developed in the old desktop-gui and applies it to the runner. There's a bit of nuance to the old implementation that implies handling of more than just the 2 prompts the way they work now. I've kept that nuance.

This PR also includes some tweaks to the header that were noticed while working on this ticket. So in Launchpad:

  • Header is now fixed to the top of the window
  • Some z-index values have been updated for modals etc to keep things correct visually

Summary of code changes

This PR:

  • makes project saved state available in gql
  • adds a mutation to set the "prompts shown" state
  • adds new project config fixtures for testing - previously in mocks, currentProject was populating config with a raw json object from the fixture file. The config now matches the array format that comes from getResolvedConfigFields so the mocks and the real app have the same format.
  • moves over existing tests around prompt behavior that weren't already covered by component tests
  • uses ci1 and orchestration1 to denote prompts for consistency
  • works around some limitations of Headless UI to meet our needs for this PR, in the long run if we which libraries for this component some of the new code here can go away. This is what a lot of the forceOpen stuff relates to
  • modifies some header styles to avoid bad text wrapping
  • improves the accessible name for the profile/log out menu

How has the user experience changed?

If it's been more than 4 days since a project was first opened, and there is not yet a cloud project ID set up, the prompt encouraging the user to set up CI will appear when the runner is launched.

How to test

  1. Remove the projectId from the cypress.json file of the project that you plan to test.
  2. Open the App (not launchpad) for that project
  3. If this projects was first opened more than 4 days ago, the prompt should appear.
  4. Dismiss the prompt in any fashion.
  5. Refresh the page.
  6. The prompt should not appear again.

To change the saved state of the project, look for the state.json files nested under /Library/Application Support/Cypress/cy/staging/projects/

To play around with other things, modify the prompts here in HeaderBarContent.vue

const prompts = sortBy([
  {
    slug: 'ci1',
    interval: interval('4 days'),
    noProjectId: true,
  },
  {
    slug: 'orchestration1',
    noProjectId: true,
  },
], 'interval')

PR Tasks

  • Have tests been added/updated?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 5, 2022

Thanks for taking the time to open a PR!

@marktnoonan marktnoonan changed the title wip changes for auto-open feat(unify): allow prompts to auto-open after set time Jan 5, 2022
@cypress
Copy link

cypress bot commented Jan 5, 2022



Test summary

4351 0 51 0Flakiness 1


Run details

Project cypress
Status Passed
Commit 8d126e3
Started Jan 10, 2022 8:47 PM
Ended Jan 10, 2022 8:57 PM
Duration 10:24 💡
OS Linux Debian - 10.10
Browser Chrome 96

View run in Cypress Dashboard ➡️


Flakiness

cypress/integration/commands/net_stubbing_spec.ts Flakiness
1 network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"

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

@marktnoonan marktnoonan marked this pull request as draft January 5, 2022 19:32
@marktnoonan marktnoonan marked this pull request as ready for review January 7, 2022 23:06
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple minor comments. There are some tests to fix up for top-nav.spec.ts. Also, have you ever noticed this before? If a test fails on CI, it looks like it doesn't run all of the suite.

Screen Shot 2022-01-10 at 1 34 04 PM

@@ -117,6 +117,18 @@ export function makeDataContext (options: MakeDataContextOptions): DataContext {
closeActiveProject () {
return openProject.closeActiveProject()
},
getCurrentProjectSavedState () {
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 make a note to bring savedState into data-context. We should be able to get this data via data-context, so the state functionality should be lifted into the ProjectLifecycleManager/config code

@@ -386,4 +414,8 @@ onKeyStroke(' ', (event) => {
resetPrompt(event)
})

onKeyStroke('Escape', (event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment above: // using onKeyStroke twice as array of keys is not supported till vueuse 6.6:. We use 7.2.2 now, can we make this simpler?

@ZachJW34 ZachJW34 merged commit ff6b2cc into 10.0-release Jan 10, 2022
@ZachJW34 ZachJW34 deleted the UNIFY-501-growth-prompt-auto-open branch January 10, 2022 21:26
tgriesser added a commit that referenced this pull request Jan 11, 2022
* 10.0-release:
  chore: change integration->e2e throughout code base (#19345)
  fix(unify): correct location of snapshot pinning UI in runner (#19640)
  fix: runnable header open in IDE and filename (#19630)
  feat(unify): allow prompts to auto-open after set time (#19571)
  test(unify): Additional navigation tests for log in workflows (#19574)
  fix(unify): minor UI bugs in the reporter (#19445)
  fix: surface openSSL unhandled rejection errors that occur within plugin code (#19598)
  feat: add spec watcher to data-context (#19583)
tgriesser added a commit that referenced this pull request Jan 13, 2022
* 10.0-release: (28 commits)
  fix: flake in shelljs scripts during postinstall (#19678)
  fix: various lifecycle issues, followup to #19572 (#19683)
  feat: error screens in app when cloud runs are failing (#18980)
  fix: open browser at correct time during lifecycle (#19572)
  chore: remove unused types (#19621)
  fix: fileparts processing and cleanup (#19660)
  chore: change integration->e2e throughout code base (#19345)
  fix(unify): correct location of snapshot pinning UI in runner (#19640)
  fix: runnable header open in IDE and filename (#19630)
  feat(unify): allow prompts to auto-open after set time (#19571)
  test(unify): Additional navigation tests for log in workflows (#19574)
  fix(unify): minor UI bugs in the reporter (#19445)
  fix: surface openSSL unhandled rejection errors that occur within plugin code (#19598)
  feat: add spec watcher to data-context (#19583)
  chore: remove unused components (#19618)
  fix: dev-server types (#19576)
  test: ct audit updates (#19464)
  chore: fix temporarily skipped specs on 10.0-release (#19611)
  fix: remove gremlins characters from cypress types
  freeze ipc
  ...
@tgriesser
Copy link
Member

@marktnoonan I know this is months after this was merged, but I just came across this code, and think we should revisit the approach here a bit. Most of the logic that exists in the client-side would probably be better managed in the GraphQL layer, with fields on Project like:

allowAutomaticPromptOpen
shouldShowCIModal
shouldShowOrchestrationModal

I know it currently works as is (though I ran into it because of a failing test elsewhere), but it's more of a bigger picture keeping where/how we expose information to the client consistent. Not a rush right now, but just wanted to mention it here. Will write up a ticket & put together a time to discuss at some point.

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.

3 participants