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

chore: Replace deprecated node-sass (#14409) #14415

Merged
merged 12 commits into from Jan 11, 2021

Conversation

kevcodez
Copy link
Contributor

@kevcodez kevcodez commented Jan 5, 2021

User facing changelog

  • Replaced deprecated node-sass with sass
  • node-sass binary is no longer required which avoids possible errors when building or including Cypress

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 5, 2021

Thanks for taking the time to open a PR!

@kevcodez
Copy link
Contributor Author

kevcodez commented Jan 5, 2021

Not sure why that one test in the npm-react build fails. Locally, all tests under npm/react run fine. Appreciate some help here.

@kevcodez kevcodez marked this pull request as ready for review January 5, 2021 17:49
@jennifer-shehane jennifer-shehane changed the title feat: Replace deprecated node-sass (#14409) chore: Replace deprecated node-sass (#14409) Jan 6, 2021
@cypress
Copy link

cypress bot commented Jan 7, 2021



Test summary

9187 0 118 3Flakiness 1


Run details

Project cypress
Status Passed
Commit aa29ab3
Started Jan 7, 2021 10:06 PM
Ended Jan 7, 2021 10:19 PM
Duration 13:19 💡
OS Linux Debian - 10.5
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/integration/retries.ui.spec.js Flakiness
1 runner/cypress retries.ui.spec > opens attempt on each attempt failure for the screenshot, and closes after test passes

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

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Some qs

@JessicaSachs
Copy link
Contributor

JessicaSachs commented Jan 8, 2021

So @kevcodez we're in a bit of a rough spot. The Percy shapshots aren't flaking... the CSS was broken. I think it was broken because the order of the css file imports changed.

A great example of this is the "Login" button under the Runs tab on the Desktop GUI (link to screenshot). But every one of these failures is real -- the padding is a little bit off here and there... the font-size is a little bit off on certain buttons (like the Login one)

I don't know what an easy answer is. The worst case is we go through screenshot-by-screenshot and write better css that doesn't rely on the order of stylesheets being compiles.

Thoughts?

@JessicaSachs JessicaSachs self-requested a review January 8, 2021 01:47
Copy link
Contributor

@JessicaSachs JessicaSachs left a comment

Choose a reason for hiding this comment

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

The Percy snapshots and some investigation show that this change reorders the CSS imports. Due to some conflicting selectors in the existing CSS, the order of the CSS imports must remain the same to prevent undesirable style changes.

@chrisbreiding
Copy link
Contributor

I wonder if putting @import 'styles/components/*.scss'; back above @import '../../ui-components/src/file-opener/file-opener'; will fix the percy snapshots: https://github.com/cypress-io/cypress/pull/14415/files#diff-fa7207623e789c59c276d8432ed09ff131907e3c2cb1ee1dc9622c683361a299

@JessicaSachs
Copy link
Contributor

@kevcodez we're gonna go ahead and fix the styles instead of playing the dangerous game of moving imports around ;-)

BTW You might not have noticed that your PR also fixes a developer-only bug that prevents us from running our packages/driver e2e suite locally. Thanks for that 👍🏻

@kevcodez
Copy link
Contributor Author

kevcodez commented Jan 8, 2021

Let me know if I can help!

I found the entire CSS setup in the desktop-gui to be quite messy. I am still working on the bootstrap 4 upgrade but it is quite hard to get it all right. This is definitely a good step in the right direction (making the CSS resilient to import order changes).

@JessicaSachs
Copy link
Contributor

JessicaSachs commented Jan 8, 2021

@kevcodez The integration tests that are failing right now seem pretty straight forward to fix if you wanted to give them a go. They were previously passing... so I don't know what changed. I will get back to this in a few days 👍🏻

BTW if you want to collaborate, here's an invite to our Discord, and we can talk all about the CSS situation

@kevcodez
Copy link
Contributor Author

kevcodez commented Jan 8, 2021

@JessicaSachs Cool, I'll have a look at the tests. Joined the discord btw (whytry#6415)

@kevcodez
Copy link
Contributor Author

kevcodez commented Jan 9, 2021

@JessicaSachs Found and fixed the issue, the react-boostrap-modal package that is used for displaying a bootstrap modal requires including a patch.css file. This file was never included. To still make the modal work, a manual overwrite of the display-style was done in the _modals.scss. I've included the patch file and removed the custom display-style.

Some Firefox tests are failing, but they seem unrelated to the changes in this PR.

@chrisbreiding chrisbreiding merged commit 0bf602c into cypress-io:develop Jan 11, 2021
@kevcodez kevcodez deleted the issue-14409 branch January 11, 2021 17:58
JessicaSachs added a commit that referenced this pull request Jan 12, 2021
Co-authored-by: Chris Breiding <chrisbreiding@gmail.com>
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Jessica Sachs <jess@jessicasachs.io>
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
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.

Replace deprecated node-sass
5 participants