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

fix: clean up some unexpected behavior and designs #21551

Merged
merged 25 commits into from Jun 21, 2022

Conversation

elevatebart
Copy link
Contributor

@elevatebart elevatebart commented May 18, 2022

While checking if we could safely remove the safelist, I found a few odd designs.

Two color class bugs

In a few places around the app, we have multiple utility classes triggering the color of the text or the background.

Since CSS classes don't take order into account, one cannot have the following code expect a deterministic rendering:

<button class="text-gray-600 text-indigo-600">
  Am I blue or gray?
</button>

It will depend on the position of both classes in the CSS file.

This use case is obvious. But when dynamic stuff is involved, it can be tricky. Look at the following vue use case.

<template>
  <button class="text-indigo-600" :class="{ 'text-gray-600': disabled }">
    Am I blue or gray?
  </button>
</template>

If the CSS class text-gray-600 is defined before text-indigo-600 in the CSS file, this button will never be gray when disabled. More deceptively, when building, the order might not be exactly the same and something that works in dev might not work in production.

Retry is not in the designs

Everywhere there is an error in Figma I only see Try again not retry.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 18, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented May 18, 2022



Test summary

37553 0 454 0Flakiness 3


Run details

Project cypress
Status Passed
Commit 38c10ae
Started Jun 21, 2022 2:27 PM
Ended Jun 21, 2022 2:43 PM
Duration 16:40 💡
OS Linux Debian - 10.11
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/actions/click.cy.js Flakiness
1 ... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
cypress/proxy-logging.cy.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second
next.cy.ts Flakiness
1 Working with next-11 > should live-reload on src changes

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

@elevatebart elevatebart changed the title refactor: try removing the safelist fix: clean up some unexpected behavior and designs May 24, 2022
@elevatebart elevatebart marked this pull request as ready for review May 24, 2022 17:26
Copy link
Contributor

@tbiethman tbiethman left a comment

Choose a reason for hiding this comment

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

Changes look good, the Percy snapshots we have show diffs I'd expect from these changes (and the existing flake). I appreciate the thorough PR description too, I'll have to keep an eye out for these scenarios going forward.

We can point this PR to develop now 🚀

@elevatebart elevatebart changed the base branch from 10.0-release to develop May 25, 2022 15:26
@marktnoonan
Copy link
Contributor

These look like good changes, gonna rerun CI for Percy.

@elevatebart
Copy link
Contributor Author

@marktnoonan when you have a minute I would love your eyes on this one.

@elevatebart elevatebart merged commit 6d34fd3 into develop Jun 21, 2022
@elevatebart elevatebart deleted the elevatebart/remove-safelist branch June 21, 2022 15:00
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 21, 2022

Released in 10.2.0.

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

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants