feat: auto-collapse sidebar for window width < lg

[MuazOthman]

• Closes

User facing changelog

Automatically collapse sidebar, and keep it collapsed, when window width is narrow to better utilize screen real estate.

Additional details

Steps to test

1. Start with a window width >=1024px, make sure the sidebar is expanded.
2. Decrease the window width to become below 1024px, the sidebar should automatically collapse.
3. As long as window width is below 1024px, the sidebar should be collapsed with no option to be expanded.
4. Going back to a window width >=1024px should automatically expand the sidebar if it was expanded before.
5. Going back to a window width >=1024px should keep the sidebar collapsed if it was collapsed before.

How has the user experience changed?

PR Tasks

• Have tests been added/updated?
• Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• Has a PR for user-facing changes been opened in cypress-documentation?
• Have API changes been updated in the type definitions?

[cypress-bot]

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]

---

Test summary

37721 • 0 • 456 • 0 • 8

---

Run details

Project	cypress
Status	Passed
Commit	03367e5
Started	Jul 16, 2022 12:02 AM
Ended	Jul 16, 2022 12:19 AM
Duration	16:45 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Flakiness

	commands/xhr.cy.js		4
1	... > logs Method, Status, URL, and XHR
2	... > logs request + response headers
3	... > logs Method, Status, URL, and XHR
4	... > logs response
	cypress/proxy-logging.cy.ts		1
1	... > intercept log has consoleProps with intercept info
This comment includes only the first 5 flaky tests. See all 8 flaky tests in the 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

[pstakoun]

Tested this out and feels great

[marktnoonan]

This does work and seems like a pretty typical pattern. I'm not sure I like taking away the user's ability to re-expand the list, even though I see why we might want to auto-close it for them. Ideally if we are auto closing something, a user can hover on the menu itself, and expand it over the content main page content so they can still pick an option. This pattern here in Vuetify is a good example. But that's a bigger UX thing and doesn't block this PR imo, since the one other place we collapse the menu it cannot be expanded by the user.

The changes needed in this PR are related to the tests, lots of tests seem to expect the nav to be expanded on these pages and will need to be adjusted before this can merge.

There's also some vertical alignment problems on the rows on this branch at smaller breakpoints, not sure what that's about, possibly it is already fixed on the main feature branch:

[marktnoonan]

@pstakoun @mike-plummer what's next for this PR?

1. We want this and we update it?
2. We don't want this right now and we close it?

[pstakoun]

@pstakoun @mike-plummer what's next for this PR?

1. We want this and we update it?
2. We don't want this right now and we close it?

@marktnoonan I would say yes, we do want this and we should update it.

[astone123]

@marktnoonan it looks like that alignment issue is being addressed here #22635

[marktnoonan]

Since our new process will need design 👀 on this PR, maybe @elevatebart or @mapsandapps can get us some confirmation ahead of time that this behavior is good to go, before we prioritize somebody cleaning up the conflicts and fixing the tests.

Thinking about it more, here's my concern:

1. The button to expand the navigation is already invisible, you have to hover over a particular place to know you can toggle the nav
2. Now there is a secret rule that will mean that at certain sizes, this button doesn't appear at all even when you hover, so things get even less discoverable

If we are going to decide for users when to expand/collapse the nav as a responsive thing, or on certain pages like the runner, maybe we should fully remove the button to toggle it, since there are now fewer situations where that's possible to do, and we override your preference anyway when we think it's better a different way. I think it's weird to have interaction patterns that are only valid some of the time, with no indication of what those times are. Better to just not have it.

But the root issue of space here could also be solved by having the means of toggling the nav be more discoverable in the first place, so if a user wants more space, they can easily see how to get it. This feedback has already been given be a few people in a more general sense. With a visible control, even if we do sometimes disable the resizing, we can also visually remove the button that triggers it, so it's always clear whether resizing is available or not.

[mapsandapps]

@marktnoonan (or whomever): where did the request & requirements for this feature come from? there's no associated ticket. (just wanting context.)

i can take this to design, yeah.

[mike-plummer]

@marktnoonan (or whomever): where did the request & requirements for this feature come from? there's no associated ticket. (just wanting context.)

i can take this to design, yeah.

@mapsandapps I think this was suggested by product and prototyped when ACI was adding the "Latest Runs" and "Average Duration" columns to the Specs Explorer - there was concern that there was insufficient screen real estate for all that at smaller viewport sizes. There's currently a "good enough" approach in place that abbreviates column headers and hides the Avg Duration column as viewport shrinks but that's probably not ideal

[mapsandapps]

approved by design ✅

[lmiller1990]

Looks like we got three ✅ but since the original branch is merged, someone will need to rebase this. The OP is off for the next little while, anyone want to rebase?

[mike-plummer]

Looks like we got three ✅ but since the original branch is merged, someone will need to rebase this. The OP is off for the next little while, anyone want to rebase?

@lmiller1990 Rebased onto develop - that might be the first time I ever managed a rebase+force-push without destroying everything ☺️

[marktnoonan]

Approved! One tiny optional suggestion on the tests.

[marktnoonan]

Hmm, should this be cy.get(SidebarSettingsLinkSelector)? Or it's probably more typical for all the others to be findByTestId('sidebar-link-settings-page'), since the selector is a test id.

[cypress-bot]

Released in 10.3.1.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.3.1, please open a new issue.