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

💥 Rename Ember specific scope option #631

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

wwilsman
Copy link
Contributor

What is this?

This SDK compensates for Ember's testing UI by utilizing a scope option to dump the contents of that scoped element into the document body. We semi-recently implemented a new type of scope option directly in our API that allows scoping snapshots to specific elements in the DOM. With this feature, the two scope options conflict, and this SDK cannot use the new element scoping feature while the Ember testing UI scope feature exists.

Rather than rename the element scoping option for this SDK only, to keep all SDKs consistent we've decided to rename the existing Ember SDK scope option to emberTestingScope. Unfortunately, that makes this a breaking change for anybody relying on the old scope option behavior. When upgrading, if not renamed, the new scope option will take effect and any cropping for that scoped element will be done by our infrastructure and might have unexpected results.

This SDK compensates for Ember's testing UI by utilizing a `scope` option to dump the contents of
that scoped element into the document body. We semi-recently implemented a new type of `scope`
option directly in our API that allows scoping snapshots to specific elements in the DOM. With this
feature, the two scope options conflict, and this SDK cannot use the new element scoping feature
while the Ember testing UI scope feature exists.

Rather than rename the element scoping option for this SDK only, to keep all SDKs consistent we've
decided to rename the existing Ember SDK scope option to `emberTestingScope`. Unfortunately, that
makes this a breaking change for anybody relying on the old `scope` option behavior. When upgrading,
if not renamed, the new `scope` option will take effect and any cropping for that scoped element
will be done by our infrastructure and might have unexpected results.
@wwilsman wwilsman added the 💥 breaking Breaking change label Aug 22, 2022
@wwilsman wwilsman requested a review from Robdel12 August 22, 2022 20:49
@wwilsman wwilsman enabled auto-merge (squash) August 22, 2022 20:53
@wwilsman wwilsman merged commit 3e01725 into master Aug 23, 2022
Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁

@delete-merged-branch delete-merged-branch bot deleted the ww/rename-ember-specific-scope-option branch August 23, 2022 16:56
@tniezurawski
Copy link

tniezurawski commented Jan 4, 2023

@wwilsman Does it mean that we should change all occurrences of scope into emberTestingScope?

When not migrating to emberTestingScope we have seen scope not working sometimes - the thing in the red rectangle should not be a part of the screenshot. Same as before, on the left, before the upgrade:

image

In some other cases, it does work. There are some changes but not as dramatic:

image


After changing scope into emberTestingScope our previous screenshots were not recognized:

image

TBH that doesn't worry me 🤷‍♂️ Most of the screenshots are fine now ☝️ but there are some problems with scope:

image

The same toolbar as above is stretched 🤔 I can work on that and investigate more but could you confirm the migration path?
scope -> emberTestingScope

Would it be worth documenting emberTestingScope somewhere here?

@itsjwala
Copy link
Contributor

itsjwala commented Jan 4, 2023

@tniezurawski could you move the details and create a new issue? We'll have a look.

Also possible to share a reproducible sample as part of the issue?

@tniezurawski
Copy link

@itsjwala Sure, I could do it but I don't know how to describe the issue 😅 First I would have to get a confirmation that upgrading to v4.0.0 requires from devs to migrate the scope option somehow. Here's how we used it so far:

await percySnapshot('Add an appointment - clients list', {
  scope: '[data-test-appointment-slide-over-dialog]',
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💥 breaking Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants