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

misc: set min widths for icons #25684

Merged
merged 8 commits into from
Feb 5, 2023
Merged

Conversation

marktnoonan
Copy link
Contributor

@marktnoonan marktnoonan commented Feb 2, 2023

Additional details

Setting min-width prevents the flex behavior from shrinking the icons.

This PR also makes another effort to reduce flake in the hover-related tests for the GroupDebugFailedTest component by calling realHover on a different element. The test failed repeatedly in CI on this branch earlier today.

Steps to test

Open the component tests for DebugFailedTest and DebugSpec component to see the new behavior at small viewports.

How has the user experience changed?

Right hand side of this Percy diff shows the corrected file icon in the top left of the Spec row:

Screen Shot 2023-02-02 at 10 59 19 AM

Right hand side of this one shows the corrected status icon on the left of the first test row

Screen Shot 2023-02-02 at 10 59 04 AM

PR Tasks

  • [na] Have tests been added/updated? - existing percy coverage captures correct new state
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@marktnoonan marktnoonan changed the title fix: set min widths for icons misc: set min widths for icons Feb 2, 2023
@cypress
Copy link

cypress bot commented Feb 2, 2023

43 flaky tests on run #43783 ↗︎

0 26782 1272 0 Flakiness 43

Details:

Fix changelog
Project: cypress Commit: b8166e8b13
Status: Passed Duration: 18:56 💡
Started: Feb 3, 2023 9:23 PM Ended: Feb 3, 2023 9:41 PM
Flakiness  e2e/origin/cookie_behavior.cy.ts • 4 flaky tests • 5x-driver-electron

View Output Video

Test
... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry
Flakiness  commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test
network stubbing > intercepting request > can delay and throttle a StaticResponse
Flakiness  e2e/origin/commands/assertions.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test
cy.origin assertions > #consoleProps > .should() and .and()
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry

The first 5 flaky specs are shown, see all 17 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

cli/CHANGELOG.md Outdated Show resolved Hide resolved
@marktnoonan marktnoonan marked this pull request as ready for review February 2, 2023 16:26
@marktnoonan marktnoonan requested a review from a team February 2, 2023 17:24
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

Just need to fix up the changelog, UI changes look good.

cli/CHANGELOG.md Outdated
@@ -11,6 +11,8 @@ _Released 02/10/2023_

- Upgraded [`simple-git`](https://github.com/steveukx/git-js) from `3.15.0` to `3.16.0` to address this [security vulnerability](https://github.com/advisories/GHSA-9p95-fxvg-qgq2) where Remote Code Execution (RCE) via the clone(), pull(), push() and listRemote() methods due to improper input sanitization was possible. Addressed in [#25603](https://github.com/cypress-io/cypress/pull/25603).

**Misc**
Copy link
Contributor

Choose a reason for hiding this comment

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

12.5.1 has already been released, you should create a new entry (I've been doing 12.6.0 since there are a few features in the queue that should be merged by next week.

You can reference 3c2b36d

@@ -7,7 +7,7 @@
size="16"
status="failed"
data-cy="failed-icon"
class="isolate"
class="min-w-16px isolate"
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not test this, but I think another option that would work is to use flex-shrink-0 to tell the flexbox not the reduce the size of this child element.

Copy link
Contributor Author

@marktnoonan marktnoonan Feb 3, 2023

Choose a reason for hiding this comment

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

I think that would work just fine too. Ideally we'll handle this in the design system. What do you think @elevatebart? Default min width to match the icon size and prevent shrinkage?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am with @warrensplayer on this one. If we want to avoid shrinking, it's better to use flex-shrink-0 if it works.
I am afraid of the side effects od doing this in the design system. specially if we want to use the size="16" model with a width of 8px for example.

@marktnoonan marktnoonan merged commit 0f7dc27 into develop Feb 5, 2023
@marktnoonan marktnoonan deleted the marktnoonan/25665-icon-size branch February 5, 2023 19:01
tgriesser added a commit that referenced this pull request Feb 8, 2023
* develop: (28 commits)
  chore: update changelog validation example (#25742)
  fix: Improve error handling around calls to `this.next` in middleware (#25702)
  chore: debug page tooltip distance and artifact border (#25727)
  fix: update newProject ref when switching between organizations in SelectCloudProjectModal (#25730)
  misc: Add max widths to debug page message states (#25725)
  chore: export types (#25714)
  chore: release @cypress/webpack-preprocessor-v5.16.3
  chore: release @cypress/vue-v5.0.4
  chore: release @cypress/grep-v3.1.4
  chore: Fix flaky test (#25726)
  dependency(deps): update dependency debug to ^4.3.4 🌟 (#25699)
  feat: openInIDE for failed debug spec (#25691)
  test: fix flaky CT test by relying on query (#25706)
  test: fix flaky migration test (#25672)
  misc: style change for responsiveness (#25687)
  misc: set min widths for icons (#25684)
  chore(deps): update dependency markdown-it to v11.0.1 🌟 (#25698)
  chore: Fix flaky origin .wait() test (#25693)
  chore: unskip tests (#25676)
  chore: release @cypress/webpack-preprocessor-v5.16.2
  ...
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.

Debug 1.1 - Prevent icons from shrinking when there is overflow
6 participants