refactor: move settings in app #18729

elevatebart · 2021-11-01T16:48:43Z

Closes https://cypress-io.atlassian.net/browse/UNIFY-524

User facing changelog

Move the "settings" page to the app
Polish all the screens connected
Wire it to all the available GraphQL data

Additional details

I also took the liberty to wire the record keys to the "empty runs" page

How has the user experience changed?

To test it

yarn dev
open the "app" project in the cypress repo
open component testing
open __vite__/#/
navigate to the settings with the left navigation bar.

Caveats

The device settings are not wired up to GQL since the gql values don't exist
specPatterns don't exist yet so the value is there just for show
experiments are not wired either

cypress-bot · 2021-11-01T16:48:46Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2021-11-01T17:34:56Z

Test summary

4803 • 0 • 52 • 2 • 1

Run details


Project	cypress
Status	Passed
Commit	`acde4aa`
Started	Nov 4, 2021 2:43 AM
Ended	Nov 4, 2021 2:55 AM
Duration	11:43 💡
OS	Linux Debian - 10.9
Browser	Electron 93

View run in Cypress Dashboard ➡️

Flakiness

	cypress/integration/commands/navigation_spec.js		1
1	src/cy/commands/navigation > #visit > window immediately resolves and doesn't reload when visiting the same URL with hashes

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

lmiller1990

Some comments!

It looks great, verified it:

lmiller1990 · 2021-11-03T07:45:47Z

packages/app/src/runs/RunsEmpty.vue

+const firstRecordKey = computed(() => {
+  const allRecordKeys = props.gql.cloudProject?.recordKeys
+
+  return allRecordKeys?.length ? allRecordKeys[0] : '<record-key>'


If you like to be concise you can do something like

return allRecordKeys?.[0] ?? '<record-key>'

Done in e7a1872

lmiller1990 · 2021-11-03T07:47:05Z

packages/app/src/settings/SettingsCard.spec.tsx

@@ -12,7 +12,7 @@ describe('<SettingsCard />', () => {

    cy.mount(() => (
      <div class="">
-        <SettingsCard title={title} description={description} icon={IconLaptop}>
+        <SettingsCard title={title} description={description} icon={IconLaptop} maxHeight="800px">


Should this just be passed as style and inherited by the first node in the SettingsCard?

I really don't want the whole style to be modifiable here. This is only an attribute meant to enable the smooth expansion of the collapsible.

What do you think?

No strong opinion, happy to go with this if you think it makes sense!

lmiller1990 · 2021-11-03T07:47:41Z

packages/app/src/settings/SettingsCard.vue

+
+<script lang="ts" setup>
+// eslint-disable-next-line no-duplicate-imports
+import type { FunctionalComponent, SVGAttributes } from 'vue'


We don't have duplicate imports, can remove this eslint disable

edit: this might be to do with script setup adding import { defineComponent } from 'vue', if so ignore this comment

I can and did remove it

lmiller1990 · 2021-11-03T07:49:46Z

packages/app/src/settings/device/ExternalEditorSettings.vue

@@ -85,5 +85,5 @@ const externalEditors = [
 ]

 const { t } = useI18n()
-const selectedEditor = ref()
+const selectedEditor = ref<Record<string, any>>()


Wouldn't this be the type defined above?

const externalEditors = [ { name: 'Visual Studio Code', key: 'vscode', icon: VSCode, },

So Record<String, Editor> or something to that meaning, where Editor is an interface with those three keys.

I agree, but I figured these types should all come from GQL anyway so I left it as the simplest iteration.

lmiller1990 · 2021-11-03T07:51:36Z

packages/app/src/settings/project/ConfigCode.vue

+  }>
+}>()
+
+// a bug in vite ddemands that we do this passthrough


Suggested change

// a bug in vite ddemands that we do this passthrough

// a bug in vite demands that we do this passthrough

lmiller1990 · 2021-11-03T07:53:41Z

packages/frontend-shared/src/components/ListRowHeader.vue

-  <div>
+  <div
+    class="flex items-center w-full text-left rounded py-12px"
+    :class="props.class"


I think we can get this for free (along with any other attrs we might add in the future, like id) with

<script lang="ts"> export default { inheritAttrs: true } </script>

I don't even think we need the inheritAttrs.
This was an artifact of when the class was passed to a lower-level item.

Removed in d6dc8a7

lmiller1990 · 2021-11-03T07:54:45Z

packages/frontend-shared/src/components/ListRowHeader.vue

    <div
-      class="flex items-center w-full text-left rounded py-12px"
-      :class="props.class"
+      v-if="$slots.right"


I think we should stick to composition API consistently, that would mean doing

const slots = useSlots()

I wonder if we can strongly type this yet 🤔

Done in 35da101

lmiller1990 · 2021-11-03T07:54:59Z

packages/frontend-shared/src/components/ListRowHeader.vue

@@ -44,5 +41,9 @@ const props = defineProps<{
  class?: string | string[] | Record<string, any>
  description?: string
  icon?: FunctionalComponent<SVGAttributes>
+  gray?: boolean
+  bigHeader?: boolean


Should this have a default value?

lmiller1990 · 2021-11-03T07:57:16Z

packages/app/cypress/e2e/integration/settings.spec.ts

+    cy.get('[href="#/settings"]').click()
+    cy.findByText('Project Settings').click()
+
+    cy.findByText('projectId')


Not sure coupling our e2e tests to the DOM hierarchy is ideal, is there really no other way to do this? Maybe just a data-cy="setting-projectid" would be more clear and less brittle.

Fixed in commit f89aab5

lmiller1990 · 2021-11-03T07:58:47Z

packages/app/package.json

+        "check-more-types",
+        "cli-cursor",
+        "cli-table3",
+        "commander",


Why did all of these get added? This really doesn't seem correct. Some may make sense but things like commander and execa and fs-extra (and a bunch of others) are Node.js only, they don't make sense here.

Probably a mistake at some point made all the node stuff go through vite. I removed it in d455ca9

marktnoonan · 2021-11-03T19:54:48Z

I added a couple of style tweaks to this branch, looks great when I run it, did not get to do a full code review.

* 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)

…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) ...

elevatebart added 10 commits October 26, 2021 11:42

refactor: move settings in app

a84ac82

add needed icons

ef6d277

icons fixes

0c71fdb

make copy-button opt in in shiki

1b21a13

almost done with projectId

53f1eb2

Merge branch 'unified-desktop-gui' into refactor/move-settings

dd549e6

Remove settingPage

819a16c

Merge branch 'unified-desktop-gui' into refactor/move-settings

ef9c920

Merge branch 'unified-desktop-gui' into refactor/move-settings

a231552

make a step forward

9097483

elevatebart added 2 commits November 1, 2021 12:01

fix: add first record key to runs empty

3587779

fix: remove double header

4f9dcea

elevatebart and others added 11 commits November 1, 2021 13:04

fix listHeader again

d59caa8

remove a change on shiki

063d6d3

fix a few issues

58f18d9

Merge branch 'unified-desktop-gui' into refactor/move-settings

d3ed66f

fix: typescript should pass

f149266

make better use of SettingsCode

dd4e33b

it is as ready as it will be

16f06b3

Merge branch 'unified-desktop-gui' into refactor/move-settings

b2dd666

dependencies optims

dff9b57

finish adding the last parts

9ca3f75

add end to end tests

f9e48e0

elevatebart marked this pull request as ready for review November 2, 2021 23:31

lmiller1990 reviewed Nov 3, 2021

View reviewed changes

elevatebart added 3 commits November 3, 2021 09:24

avoid using the DOM structure to end 2 end test

f89aab5

more concise ternary

e7a1872

two fixes in ListRowHeader.vue

35da101

elevatebart and others added 9 commits November 3, 2021 09:45

remove unwanted vite dependencies

d455ca9

Merge branch 'unified-desktop-gui' into refactor/move-settings

cfee768

strengthen typings

72db119

remove explicit class prop from RowHeader

d6dc8a7

fix some accesibility issues

07d22d3

restore focused status

5fd8d11

rename runspage to runscontainer

5de40ec

minor css tweaks

3969094

match some border values to figma

a06a9e6

lmiller1990 added 2 commits November 4, 2021 11:47

merge in origin

29de471

resolve conflicts

acde4aa

lmiller1990 merged commit 5f77c15 into unified-desktop-gui Nov 4, 2021

lmiller1990 deleted the refactor/move-settings branch November 4, 2021 02:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move settings in app #18729

refactor: move settings in app #18729

elevatebart commented Nov 1, 2021 •

edited

Loading

cypress-bot bot commented Nov 1, 2021

cypress bot commented Nov 1, 2021 •

edited

Loading

lmiller1990 left a comment

lmiller1990 Nov 3, 2021

elevatebart Nov 3, 2021 •

edited

Loading

lmiller1990 Nov 3, 2021

elevatebart Nov 3, 2021

lmiller1990 Nov 4, 2021

lmiller1990 Nov 3, 2021

elevatebart Nov 3, 2021

lmiller1990 Nov 3, 2021

elevatebart Nov 3, 2021

lmiller1990 Nov 3, 2021

lmiller1990 Nov 3, 2021

elevatebart Nov 3, 2021

lmiller1990 Nov 3, 2021

elevatebart Nov 3, 2021 •

edited

Loading

lmiller1990 Nov 3, 2021

lmiller1990 Nov 3, 2021

elevatebart Nov 3, 2021

lmiller1990 Nov 3, 2021

elevatebart Nov 3, 2021 •

edited

Loading

marktnoonan commented Nov 3, 2021

	// a bug in vite ddemands that we do this passthrough
	// a bug in vite demands that we do this passthrough

refactor: move settings in app #18729

refactor: move settings in app #18729

Conversation

elevatebart commented Nov 1, 2021 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

To test it

Caveats

cypress-bot bot commented Nov 1, 2021

cypress bot commented Nov 1, 2021 • edited Loading

Test summary

Run details

Flakiness

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elevatebart Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elevatebart Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elevatebart Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

marktnoonan commented Nov 3, 2021

elevatebart commented Nov 1, 2021 •

edited

Loading

cypress bot commented Nov 1, 2021 •

edited

Loading

elevatebart Nov 3, 2021 •

edited

Loading

elevatebart Nov 3, 2021 •

edited

Loading

elevatebart Nov 3, 2021 •

edited

Loading