-
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
feat(app): support editor preference #18932
Conversation
@@ -8,11 +8,11 @@ import '../main.scss' | |||
/* global cy, Cypress */ | |||
describe('FilePreference', () => { | |||
const availableEditors = [ | |||
{ id: 'atom', name: 'Atom', isOther: false, openerId: 'atom' }, |
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.
openerId
and binary
are the same, but duplicated. Removing this is technical a breaking change, so now is the time to do it. Worst case scenario is the user will need to select their preferred editor again.
Test summaryRun details
View run in Cypress Dashboard ➡️ Failures
Flakiness
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 |
id: 'other', | ||
name: 'Other', | ||
openerId: '', | ||
isOther: true, |
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.
Removed this option from here - it's only here for the purpose of having a placeholder on the front-end UI.
We still need that placeholder for now, so I append it here... https://github.com/cypress-io/cypress/pull/18932/files#diff-164ce6bedd1fe744fbd75f7b8526d850c94eb6fd43b9c22e60b18e976d9b21a8R376 will delete post 10.0.
@@ -67,37 +66,17 @@ describe('lib/util/editors', () => { | |||
sinon.restore() | |||
}) | |||
|
|||
it('returns a list of editors on the user\'s system with an "On Computer" option prepended and an "Other" option appended', () => { |
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.
No longer needed, we no long append this option in editors.ts
, instead we do it elsewhere. I don't think this placeholder, which is here for the purpose of having a placeholder in the UI, should be the responsibility of this server module.
More info: https://github.com/cypress-io/cypress/pull/18932/files#r750277563
}, | ||
}) | ||
|
||
t.liveMutation('setWatchForSpecChange', { |
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.
Doesn't matter for this PR, but just wanted to mention I think it might be better if we start moving away from the liveMutation
everywhere, now that we have patterns for state management nailed down in our urql cache, linting on ensuring id
exists, etc. Changing a config setting probably doesn't need to refetch everything on the page if we know it's updating a single field.
Was going to introduce that in #18906 and document in the general architecture guide, but don't worry about making that change here
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.
Sure, lets's get rid of live mutation in one single PR - sounds good
@@ -300,6 +300,54 @@ export const mutation = mutationType({ | |||
}, | |||
}) | |||
|
|||
t.liveMutation('setAutoScrollingEnabled', { |
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 might be simpler to have a single mutation for this like updateSettings($input: UpdateSettingsInput)
. We can change that later though
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.
Yeah just realized this, I want to redo the entire cache layer (for appData) and probably add this refactor then (as well as typing the rest of the appData layer etc...)
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.
cy.mount(() => <ExternalEditorSettings class="p-12" />) | ||
cy.mountFragment(ExternalEditorSettingsFragmentDoc, { | ||
onResult: (ctx) => { | ||
ctx.localSettings.availableEditors = [ |
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 this be stubbed in packages/frontend-shared/cypress/support/mock-graphql/stubgql-*
?
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.
gql` | ||
mutation SetPreferredEditorBinary ($value: String!) { | ||
setPreferredEditorBinary (value: $value) | ||
|
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.
Remove newline
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.
return props.gql.localSettings.availableEditors.find((x) => x.binary === props.gql.localSettings.preferences.preferredEditorBinary) | ||
}) | ||
|
||
const updateEditor = async (editor: ExternalEditorSettingsFragment['localSettings']['availableEditors'][number]) => { |
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.
Is there any reason to use async here?
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.
export class LocalSettingsActions { | ||
constructor (private ctx: DataContext) {} | ||
|
||
async setDevicePreference<K extends keyof DevicePreferences> (key: K, value: DevicePreferences[K]) { |
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.
Do we need async?
@@ -0,0 +1,5 @@ | |||
import type { DataContext } from '..' | |||
|
|||
export class LocalSettingsSource { |
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.
Do we need this? And if we do, it should be named LocalSettingsDataSource
.
…e-data-clean-refactor * tgriesser/chore/e2e-data-clean: (76 commits) chore: post-merge cleanup feat: use hoisted yarn install in binary build (#17285) fix: fix spec list header, "Create specs" prompt, add workspace recommended apollo extension (#18993) feat(unify): reporter settings (#18946) feat: add devServer to config file (#18962) fix: compile npm packages for node 12 (#18989) fix: show call count even if `cy.stub().log(false)`. (#18907) chore: Update TypeScript to 4.4.4 (#18930) feat: use fuzzy search (#18966) fix: onUnmounted warning in topnav (#18988) fix: wrap playground selectors in double quotes if not included (#18442) fix: flaky settings_spec test (#18979) fix: CYPRESS_INTERNAL_VITE_DEV for development feat: Create default config file (#18943) feat(app): support editor preference (#18932) chore: Update Chrome (stable) to 96.0.4664.45 (#18931) fix: Loading of specs with % in the filename (#18877) feat: improve vite DX (#18937) chore: refactor `create` into class `$Cy` (#18715) feat: Use plugins on config files (#18798) ...
saved_state
caching will all move to data-context layer, but it's too big to do right now)All the data here is wired up to the server and working correctly: