fix: various Nav Bar fixes #19283

elevatebart · 2021-12-06T20:55:53Z

Closes UNIFY-620

What has changed

Coco.mov

User facing changelog

Nav Bar is now collapsible on runs
Nav Bar now has the height of the screen
In the layout Main automatically scrolls when needed
Navigation only uses the name of the page component file
Runner is now a sub-route of specs
When opening the specs page navigation is collapsed and the button to uncollapse it is hidden
When leaving specs, everything comes back to normal

cypress-bot · 2021-12-06T20:55:56Z

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-12-06T21:48:58Z

Test summary

18147 • 0 • 202 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`b7931d2`
Started	Dec 8, 2021 5:05 PM
Ended	Dec 8, 2021 5:17 PM
Duration	11:50 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

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

Seems fine, two comments, I don't see specific reason to use _ for Pinia state. Also check-ts is busted.

lmiller1990 · 2021-12-06T23:26:22Z

packages/app/src/specs/generators/component/ComponentGeneratorStepOne.vue

    >
      <router-link
        class="outline-none"
-        :to="{ path: 'runner', query: { file: result.spec.relative } }
+        :to="{ path: 'specs/runner', query: { file: result.spec.relative } }


should this be absolute like the rest?

agreed

Suggested change

:to="{ path: 'specs/runner', query: { file: result.spec.relative } }

:to="{ path: '/specs/runner', query: { file: result.spec.relative } }

lmiller1990 · 2021-12-06T23:26:59Z

packages/app/src/store/main-store.ts

@@ -13,12 +14,19 @@ export const useMainStore = defineStore({
  id: 'main',
  state: (): MainStoreState => {
    return {
-      navBarExpanded: true,
+      _navBarExpanded: true,


Why is this prefixed with _? Seems to serve no purpose, it's not really a private variable.

It has to have a different name than the exposed variable and should never be updated on it's own.
How else can I achieve this private state member in pinia?

I renamed it so that the flag can be set in tests.

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

ZachJW34

There is an issue when changing browsers and preserving the runnable spec. You can test that it is broken by selecting a spec and then switching to another browser. The backend redirects the frontend incorrectly with this change.

I made a PR to fix this here: #19290

elevatebart · 2021-12-07T16:50:19Z

@ZachJW34 Thank you, I merged your fix.
I am not super sure I like the original architecture.

ZachJW34 · 2021-12-07T17:29:43Z

@elevatebart Me neither, I don't think the backend should know/care about the frontend routing. Maybe a redirect on the frontend when matching a certain pattern could help out a bit. I'll take note of it.

ZachJW34

Can we remove setNavBarExpandedByUser? It's only being used in a spec.

Also, I think this state should be removed since it makes me think that the nav-bar can be expanded when the runner is open, but clicking it does nothing

lmiller1990 · 2021-12-07T21:41:42Z

@ZachJW34 it's used in the getter https://github.com/cypress-io/cypress/pull/19283/files#diff-36948646cccceb78ddb20bb13a2a2ca2c89c6907841c930fdf044c75eae56cd8R32

changes made

marktnoonan · 2021-12-07T22:30:50Z

My only comment on the nav is the same as @ZachJW34, to not have the "expand" control appear on hover if the route does not allow the navigation to be expanded.

I think there is a slight miscommunication between @lmiller1990 and Zach - Zach’s talking about the setter for navBarExpandedByUser and I think Lachlan you’re talking about navBarExpandedByUser itself.

ZachJW34

Tested, works great. LGTM!

…text * 10.0-release: (45 commits) fix: various Nav Bar fixes (#19283) build: add patch package as a dev dependency for fe-shared chore: hoist is - fun with cached dependencies build: hoist is hard build: better hoisting strategy fix: remove windows and mac workflow from branch revert: remove change about node version 17 build: remove testing of desktop-gui assets build: run window & mac CI in this branch build: more fixes build: remove toycode mdi from launchpad rename patch because of dev dep build: fix merge issue in packages generation chore: update sass for windows compatibility fix: Do not crash when a ill formed URL request is proxied (#19274) fix: remove desktop-gui from circle.yml change whitepace in patch fix: adding timeout option to writeFile command (#19015) release 9.1.1 fix: patch-package is not applied in dist'ed build (#19239) ...

* 10.0-release: (53 commits) refactor: makeLegacyContext -> getCtx (#19308) fix: various Nav Bar fixes (#19283) build: add patch package as a dev dependency for fe-shared chore: hoist is - fun with cached dependencies build: hoist is hard build: better hoisting strategy chore: remove unused testing preferences (#19301) fix: remove windows and mac workflow from branch fix: show script errors when spec file fails to process (#19298) revert: remove change about node version 17 feat: open config file in user's editor (#19276) feat(unify): scale the AUT (#19297) build: remove testing of desktop-gui assets build: run window & mac CI in this branch build: more fixes build: remove toycode mdi from launchpad rename patch because of dev dep build: fix merge issue in packages generation chore: update sass for windows compatibility fix: Do not crash when a ill formed URL request is proxied (#19274) ...

elevatebart added 5 commits December 6, 2021 11:37

fix: makenav bar collapsible on runs

5acb080

fix: make runner a subpage of specs

356620a

fix: make runner page always collapsed

67d2363

fix: cleanup routes meta tags

0d987d0

fix: hide collapser when no collapse

11d6fb4

elevatebart and others added 3 commits December 6, 2021 15:16

fix: routing metas

493edb6

fix: capitalize the files

8de2e2d

Merge branch '10.0-release' into elevatebart/fix/nav-bar

480bae3

elevatebart marked this pull request as ready for review December 6, 2021 21:30

fix: make layout scroll both ways

87ab24f

refactor: remove css code and use tailwind instead

3d0496e

lmiller1990 reviewed Dec 6, 2021

View reviewed changes

elevatebart and others added 4 commits December 6, 2021 19:07

fix: make all path absolute

44bbfbc

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

fix: rename flag to expand the nav

8db373c

Merge branch '10.0-release' into elevatebart/fix/nav-bar

05813a1

Merge branch '10.0-release' into elevatebart/fix/nav-bar

fc5c8de

ZachJW34 self-requested a review December 7, 2021 15:25

ZachJW34 previously requested changes Dec 7, 2021

View reviewed changes

fix: preserve runnable spec on browser change (#19290)

d5a0175

ZachJW34 self-requested a review December 7, 2021 17:35

refactor: fixes on the main-store

5d63a38

ZachJW34 reviewed Dec 7, 2021

View reviewed changes

lmiller1990 previously approved these changes Dec 7, 2021

View reviewed changes

elevatebart and others added 4 commits December 7, 2021 16:47

Merge branch '10.0-release' into elevatebart/fix/nav-bar

4cbb82a

Merge branch '10.0-release' into elevatebart/fix/nav-bar

9ec2c69

Merge branch '10.0-release' into elevatebart/fix/nav-bar

3afc118

test: editor settings e2e test fixed

3418f06

elevatebart dismissed lmiller1990’s stale review via 3418f06 December 8, 2021 16:33

fix: the bug zach found

b7931d2

elevatebart force-pushed the elevatebart/fix/nav-bar branch from 802cb1d to b7931d2 Compare December 8, 2021 16:59

ZachJW34 approved these changes Dec 8, 2021

View reviewed changes

elevatebart merged commit 33233e9 into 10.0-release Dec 8, 2021

elevatebart deleted the elevatebart/fix/nav-bar branch December 8, 2021 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: various Nav Bar fixes #19283

fix: various Nav Bar fixes #19283

elevatebart commented Dec 6, 2021 •

edited

cypress-bot bot commented Dec 6, 2021

cypress bot commented Dec 6, 2021 •

edited

lmiller1990 left a comment

lmiller1990 Dec 6, 2021

elevatebart Dec 6, 2021

lmiller1990 Dec 6, 2021

elevatebart Dec 7, 2021

elevatebart Dec 7, 2021

ZachJW34 left a comment

elevatebart commented Dec 7, 2021

ZachJW34 commented Dec 7, 2021

ZachJW34 left a comment

lmiller1990 commented Dec 7, 2021

marktnoonan commented Dec 7, 2021

ZachJW34 left a comment

	:to="{ path: 'specs/runner', query: { file: result.spec.relative } }
	:to="{ path: '/specs/runner', query: { file: result.spec.relative } }

fix: various Nav Bar fixes #19283

fix: various Nav Bar fixes #19283

Conversation

elevatebart commented Dec 6, 2021 • edited

What has changed

User facing changelog

cypress-bot bot commented Dec 6, 2021

cypress bot commented Dec 6, 2021 • edited

Test summary

Run details

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 Dec 6, 2021

Choose a reason for hiding this comment

elevatebart Dec 6, 2021

Choose a reason for hiding this comment

lmiller1990 Dec 6, 2021

Choose a reason for hiding this comment

elevatebart Dec 7, 2021

Choose a reason for hiding this comment

elevatebart Dec 7, 2021

Choose a reason for hiding this comment

ZachJW34 left a comment

Choose a reason for hiding this comment

elevatebart commented Dec 7, 2021

ZachJW34 commented Dec 7, 2021

ZachJW34 left a comment

Choose a reason for hiding this comment

lmiller1990 commented Dec 7, 2021

marktnoonan commented Dec 7, 2021

ZachJW34 left a comment

Choose a reason for hiding this comment

elevatebart commented Dec 6, 2021 •

edited

cypress bot commented Dec 6, 2021 •

edited