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

Add percy #1296

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add percy #1296

wants to merge 4 commits into from

Conversation

mansona
Copy link
Collaborator

@mansona mansona commented Nov 1, 2024

No description provided.

Copy link

netlify bot commented Nov 1, 2024

Deploy Preview for ember-paper ready!

Name Link
🔨 Latest commit 33baac5
🔍 Latest deploy log https://app.netlify.com/sites/ember-paper/deploys/6728b562949dd30008efd5b3
😎 Deploy Preview https://deploy-preview-1296--ember-paper.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Nov 4, 2024

Some tests with 'continue-on-error: true' have failed:

@mansona
Copy link
Collaborator Author

mansona commented Nov 4, 2024

@matthewhartstonge I would like to get your opinion on this one. I noticed when I added percy that it wasn't a full-screen snapshot. It turns out that a few decisions were made in the past that prevented that.

This PR fixes it but it is actually a breaking change 🙈 I'm wondering if you have any thoughts on what we should do here?

Here is the percy build with a before and after my last commit: https://percy.io/Ember/web/ember-paper/builds/37201488/changed/2027196211

@mansona mansona self-assigned this Nov 4, 2024
Copy link
Contributor

@matthewhartstonge matthewhartstonge left a comment

Choose a reason for hiding this comment

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

I've run a test with percy @ #1297

I've simplified it to only lock if in the dummy app on the catalog route to (hopefully) remove the breaking change. Does that satisfy backwards compat?

I saw dialogs remove scrolling, which becomes an issue with the custom dialogs that run off the screen.

toastDuration = 60000;
// Toast duration needs to be falsey so that we can take a snapshot with percy
// TODO we might be able to clean this up later when we remove use of `later()` in the codebase
toastDuration = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

ah cool! didn't know we could set this 😂

@@ -6,6 +6,7 @@ import { inject as service } from '@ember/service';
import { faker } from '@faker-js/faker';
import { tracked } from '@glimmer/tracking';
import { buildGridModel } from '../utils/grid-list';
import { isTesting } from '@embroider/macros';
Copy link
Contributor

Choose a reason for hiding this comment

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

neato! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants