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: Updating reporter panel's default width to match new designs #20231

Merged
merged 10 commits into from
Feb 21, 2022

Conversation

tbiethman
Copy link
Contributor

@tbiethman tbiethman commented Feb 17, 2022

Additional details

New designs put the default reporter panel width at 450px, up from 320px.

How has the user experience changed?

Old:
Screen Shot 2022-02-16 at 11 10 31 PM

New:
Screen Shot 2022-02-16 at 11 08 39 PM

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?
  • Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 17, 2022

Thanks for taking the time to open a PR!

assertWidth('panel1', 1000)
assertWidth('panel3', 500)
dragHandleToClientX('panel1', 950)
dragHandleToClientX('panel1', 955)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marktnoonan I was surprised that this test started failing due to panel 1 no longer resizing to 1000px. But it turns out we still account for panel 2's width when deciding panel 1's maximum size, even when panel 2 is hidden. It doesn't look like we disable panel 2 anywhere but the tests, but I'm curious if this behavior is intentional.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true in the app there's no case where you show the inline specs list but hide the reporter, so this doesn't make a difference from the UI. It might be cleanest/easiest to maintain to just fix the component to behave in a more expected way even though we don't currently need that.

We can also now test the real UI scenarios from Cypress-In-Cypress, which wasn't possible when this component was created, so that will be a good addition to our Spec Runner tests. We might need a ticket if this isn't already on the radar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed on the additional tests, I'll log something to track this. We'll want get a bunch of workflows snapshotted once our Percy workflows are more stable.

As far as the showPanel2 prop goes, do we even want to continue supporting it? Or do we anticipate a need in the near future?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think we want to selectively show/hide panel 2 in certain screenshot configurations, I have a ticket to roll through the whole cy.screenshot API. If it turns out we don't need it, we can remove the ability.

@tbiethman tbiethman changed the title feat: Updating reporter panel's default width to match designs feat: Updating reporter panel's default width to match new designs Feb 17, 2022
@tbiethman tbiethman self-assigned this Feb 17, 2022
@cypress
Copy link

cypress bot commented Feb 17, 2022



Test summary

17990 0 210 0Flakiness 4


Run details

Project cypress
Status Passed
Commit 60210e2
Started Feb 21, 2022 6:06 PM
Ended Feb 21, 2022 6:19 PM
Duration 12:16 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/commands/net_stubbing.cy.ts Flakiness
1 network stubbing > intercepting request > can delay and throttle a StaticResponse
2 network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
3 network stubbing > intercepting response > can 'delay' a proxy response using Promise.delay
4 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 previously approved these changes Feb 18, 2022
Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

LGTM. From a UX perspective I have some reservations about how this will work on smaller windows, but I'll bring that up separately, doesn't block this change.

@@ -782,11 +782,15 @@ describe('src/cy/commands/actions/trigger', () => {

describe('position argument', () => {
Copy link
Contributor Author

@tbiethman tbiethman Feb 18, 2022

Choose a reason for hiding this comment

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

With the change to the default width, the AUT is scaled differently as the third panel now has less room. Firefox behavior varies in how the AUT scrolls at different scales when the buttons are injected into the DOM for these tests, and that impacts the assertions we are making regarding client coordinates. You can see where this was a problem for the existing size at lines 828/846.

I updated the other tests that were being made that are impacted by this variability. I also left comments and tried to make the meaning of the numbers in the calculations a little more clear. These calculations match those done here.

@tbiethman tbiethman merged commit 27fafde into 10.0-release Feb 21, 2022
@tbiethman tbiethman deleted the UNIFY-1168-reporter-width branch February 21, 2022 19:28
tgriesser added a commit that referenced this pull request Feb 22, 2022
* 10.0-release:
  feat: Updating reporter panel's default width to match new designs (#20231)
  feat(unify): Login error states (#20204)
tgriesser pushed a commit that referenced this pull request Feb 22, 2022
parent 6279a7d
author Brian Mann <brian.mann86@gmail.com> 1644881743 -0500
committer Tim Griesser <tgriesser10@gmail.com> 1645497717 -0500

chore: post merge cleanup

add visual tests for new errors, update formatting + copy of existing errors to bring into parity with new conventions

fix require, lazy load if necessary

export what is called directly to avoid circular ref proxy that's unstubbable

export additional properties on error, update refs, fix tests

fix missing ref

rename clone -> cloneErr

use proper method call

fix: types for config changes

fix: clean up a few test failures

fix: additional cleanup of errors following merge

fix: cleanup a few more tests/snapshot errors

feat: reconfigure project from testing type card (#20109)

fix: remove inner scroll from BaseError (#20104)

feat: package manager in update modal (#20123)

fix: Backing out driver change for type (#20155)

* fix: Backing out driver change for type

* Adding comment for posterity

* Fixing test

feat: detect component testing env (#20002)

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

fix: use github.com qualified path to deal with yarn cache bug (#20158)

feat: Update version checking logic to query the manifest and ensure ctx.util.fetch can handle a proxy (#20107)

Co-authored-by: Zach Bloomquist <git@chary.us>

fix: cleanup integration folder after moving specs to e2e (#20149)

Co-authored-by: Barthélémy Ledoux <bart@cypress.io>
Co-authored-by: ElevateBart <ledouxb@gmail.com>

fix: Remove mention of Cypress Studio from Experimental Project Settings (#20154)

fix: remove phantom migration watchers (#20180)

fix: migration ui differences (#20134)

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>
Co-authored-by: Barthélémy Ledoux <bart@cypress.io>

test: use async copy for scaffolding project (#20181)

fix: migration on windows (#20063)

Co-authored-by: Zach Bloomquist <git@chary.us>
Co-authored-by: Ryan Manuel <ryanm@cypress.io>

test: clean up scaffoldMigrationProject (#20203)

fix: rename folder instead of skip on migration (#20145)

Co-authored-by: Emily Rohrbough  <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

chore: ignore checksums when installing Chrome in Windows CI (#20210)

feat(unify): inline specs list tweaks (#20157)

test: fix some windows tests (#20225)

test(unify): Settings E2E Tests (#20126)

Co-authored-by: Zachary Williams <ZachJW34@gmail.com>

fix: migration hover/focus states (#20243)

Co-authored-by: ElevateBart <ledouxb@gmail.com>

fix: refactor function getFilesByGlob to use concat instead of isArray (#20245)

* fix: avoid using Array.isArray when possible

* fix: add one more refactoring

Co-authored-by: Zachary Williams <ZachJW34@gmail.com>

Co-authored-by: Zachary Williams <ZachJW34@gmail.com>

test: Migrate more runner UI specs to app (#20118)

* Adding remaining reporter error tests.

* Removing existing integration spec file

* Removing existing runner fixtures for error specs

* Fixing ts lint

* Testing with less tests kept in memory

* Testing some cypress-in-cypress settings to improve run-mode performance.

* Reverting changes to doc_url specs

* PR Updates. Inverting default value for 'open in IDE' validation in an attempt to reduce test time.

* Cleaning up ported code

* Update system-tests/projects/runner-e2e-specs/cypress/fixtures/index.html

Co-authored-by: Emily Rohrbough  <emilyrohrbough@users.noreply.github.com>

* Trying build workaround

* Bumping up parallelized builds for app integration tests

* Removing suspicious tests to validate CI build

* Removing more suspicious tests to test CI

* Trying to not scaffold every navigation

* Bumping resources for run-app-integration job. Updating reporter.errors specs to appropriately reduce AUT refreshes.

* Couple API tweaks

* test: Migrating runner hooks specs to app

* Removing unused helper from runner package; cleaning up a few things

* Updating yarn.lock

* fix: Migrating runner sessions ui tests to app

* test: Migrating runner's runner.ui tests to app

* test: Migrating runner retries ui tests to app

* Cleaning up a few things for consistency

* Fixing ts errors

* Removing tests for runner header that have existing coverage in app

* Working around detached elements in CI

* Still working around detached elements in CI

* Updating sessions.ui test to click more carefully. Adding bit more coverage around runner header and snapshot pinning.

* Update packages/app/cypress/e2e/runner/retries.ui.cy.ts

Co-authored-by: Ryan Manuel <ryanm@cypress.io>

* Migrating issue specs. Updating TODOs. Hopefully addressing some flake.

* Stopping icon spin animation for percy snapshots

* WIP

* customPercyCommand.ts

* WIP: working on custom percy snapshots for hiding elements other than the reporter [skip ci]

* WIP: Adding naive mutation observer for reseting DOM mutations after snapshots [skip ci]

Got tired of tripping over types, I can add them when we're feature complete

* Implementing scoped snapshots throughout app/runner tests, let's see how percy output looks

* Adding more snapshots to runner tests.

* Using spec link as click target

* Stop taking snapshots of failed tests

* Scoping selector playground snapshots (though they're still unstyled)

* Addressing flake in virtualized spec list

* Drying up runner snapshotting

* Logging single percy snapshot that reflects proper viewport and overrides

* Updating describes to better match previous snapshot names

* Removing added snapshots to get more accurate better diff presentation in Percy

* Removing new runner.ui snapshots as well

* Adding a few PR recommendations. Updating one last spec name that was corrected for grammar and no longer matched. Updating/removing relevant TODOs.

* Updating to use anticipated default width for reporter

* Updating tests for consistency with previous snapshots

* Tying original command into promise chain

* Let's try that again

* Making percy command enqueuing explicit to test result

* Scoping down on some variables. Using defaults for sessions tests.

* Updating some overrides to see if visibility is handled inconsistently

* Whitespace vanquished, working session test updates back in for long command logs

* Viewport height is not reflected in snapshots, let's try forcing the height on the panel

* Forcing the height issue

* Let's try a minHeight on the percy command

* Cleaning up alterations made during percy exploration

* Trying to get a cleaner percy diff after merge

* Fix Percy to use 10.0-release branch as baseline image comparison

* add comments about temporary percy env vars

* Adding some doc for changes made to customPercyCommand

Co-authored-by: Emily Rohrbough  <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Ryan Manuel <ryanm@cypress.io>
Co-authored-by: Brian Mann <brian.mann86@gmail.com>
Co-authored-by: Emily Rohrbough <emilyrohrbough@yahoo.com>

feat(unify): show spec runner header in run mode (#20189)

Co-authored-by: Emily Rohrbough  <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Zachary Williams <ZachJW34@gmail.com>
Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

fix: connect to dashboard page when there are no existing projects (#20188)

Co-authored-by: Barthélémy Ledoux <bart@cypress.io>

chore: fix system-test-firefox screenshots_spec flake (#20260)

fix all the tests that escape "doesn\'t" to use string templates

fix test assertion

fix test importing wrong strip_indent

fix snapshot

remove accidental system test it.only

properly skip describe suites + tests and add TODO comments

export errors.theme, update imports not to require from deep inside packages

export stripAnsi onto the old errors object

fix yarn.lock

chore: remove system test it.only and prevent CI from ever running onlys (#20276)

* remove accidental system test it.only

* prevent system tests from running in CI if set to run exclusively with it.only(...)

chore: fix server performance flake (#20280)

* chore: fix server performance flake

* rerun in circle

Co-authored-by: Brian Mann <brian.mann86@gmail.com>

fix invalid exports

remove fixtures after shutting down the project

add terminals app watch command

updated snapshots, added tests to cover --spec CLI arg + custom specPattern config

- removed unused system project

update deep import of @packages/errors

fix root system test & launchpad choose-a-browser spec

fix: skip test that is no longer correct in 10.x

fix: serialize details when serializing the error

remove plugins file not found error/test

update CONFIG_FILE_REQUIRE_ERROR

working through mapping error cases from develop

Fixing CONFIG_VALIDATION_ERROR / SETUP_NODE_EVENTS_IS_NOT_FUNCTION errors

fix remaining test failures

fix spec failures

Attempt to fix awesome-typescript-loader failures

fix snapshot

feat(unify): Login error states (#20204)

feat: Updating reporter panel's default width to match new designs (#20231)

* feat: Updating default reporter width to 450px

* Updating component tests to account for new default panel 2 width

* Giving the cypress-in-cypress tests a little more room to breath

* Updating trigger test coordinate assertions to be have consistent results regardless of AUT scaling

* Updating scale assertion now that available width has changed
tgriesser added a commit that referenced this pull request Feb 22, 2022
* 10.0-release: (29 commits)
  feat: Updating reporter panel's default width to match new designs (#20231)
  feat(unify): Login error states (#20204)
  chore: fix server performance flake (#20280)
  chore: remove system test it.only and prevent CI from ever running onlys (#20276)
  chore: fix system-test-firefox screenshots_spec flake (#20260)
  fix: connect to dashboard page when there are no existing projects (#20188)
  feat(unify): show spec runner header in run mode (#20189)
  test: Migrate more runner UI specs to app (#20118)
  fix: refactor function getFilesByGlob to use concat instead of isArray (#20245)
  fix: migration hover/focus states (#20243)
  test(unify): Settings E2E Tests (#20126)
  test: fix some windows tests (#20225)
  feat(unify): inline specs list tweaks (#20157)
  chore: ignore checksums when installing Chrome in Windows CI (#20210)
  fix: rename folder instead of skip on migration (#20145)
  test: clean up scaffoldMigrationProject (#20203)
  fix: migration on windows (#20063)
  test: use async copy for scaffolding project (#20181)
  fix: migration ui differences (#20134)
  fix: remove phantom migration watchers (#20180)
  ...
tgriesser added a commit that referenced this pull request Feb 22, 2022
* 10.0-release: (29 commits)
  feat: Updating reporter panel's default width to match new designs (#20231)
  feat(unify): Login error states (#20204)
  chore: fix server performance flake (#20280)
  chore: remove system test it.only and prevent CI from ever running onlys (#20276)
  chore: fix system-test-firefox screenshots_spec flake (#20260)
  fix: connect to dashboard page when there are no existing projects (#20188)
  feat(unify): show spec runner header in run mode (#20189)
  test: Migrate more runner UI specs to app (#20118)
  fix: refactor function getFilesByGlob to use concat instead of isArray (#20245)
  fix: migration hover/focus states (#20243)
  test(unify): Settings E2E Tests (#20126)
  test: fix some windows tests (#20225)
  feat(unify): inline specs list tweaks (#20157)
  chore: ignore checksums when installing Chrome in Windows CI (#20210)
  fix: rename folder instead of skip on migration (#20145)
  test: clean up scaffoldMigrationProject (#20203)
  fix: migration on windows (#20063)
  test: use async copy for scaffolding project (#20181)
  fix: migration ui differences (#20134)
  fix: remove phantom migration watchers (#20180)
  ...
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