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

US156331 - Add ability to indicate the actual element to use for size calculations for the screenshot #146

Merged
merged 6 commits into from
Aug 25, 2023

Conversation

svanherk
Copy link
Contributor

@svanherk svanherk commented Aug 24, 2023

Some root custom elements that consumers pass to vdiff won't be the one that defines its true size. For example, the <d2l-dialog> element has no height or width -- instead, the size is defined by an element in its shadow root. As a result, snapshotting a dialog fails.

This PR adds functionality to look for the "real" size. If your custom element is one of these, you can add the vdiff-size vdiff-target class to the element in its shadowRoot that does have the proper size. This will search recursively, until there's no element in the shadowRoot with vdiff-size vdiff-target or we reach a native HTML element. I've added a couple tests to demonstrate this.

Not tied to any of the naming, so suggest away!

Without the changes to check for vdiff-size vdiff-target, the new goldens here would look like this:
image

@svanherk svanherk requested a review from a team as a code owner August 24, 2023 17:18
}
render() {
return html`
<div class="vdiff-size">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This represents typical core usage. We'll add the appropriate class to dialog, dropdown`, etc.

class extends LitElement {
render() {
return html`${unsafeHTML(`
<${fixedElementTag} class="vdiff-size"><${elementTag} text="Visual Difference"></${elementTag}></${fixedElementTag}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This represents what is likely to happen for consumers of core. If they have a component that wraps d2l-dialog for example, they'll add the class to d2l-dialog. vdiff will look for this class, then follow it into d2l-dialog to use the element we've added the class to.

@@ -196,6 +196,7 @@ export class WTRConfig {
config.groups.forEach(g => g.browsers = this.getBrowsers(g.browsers));

if (open || watch) {
config.testsFinishTimeout = 15 * 60 * 1000;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit unrelated, but when I was trying to debug my new tests with --open, I only had two minutes before the whole thing force-closed. This ups that timeout to 15 minutes in open or watch mode.

Copy link
Member

@dlockhart dlockhart left a comment

Choose a reason for hiding this comment

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

Seems straightforward enough! Not sure about the CSS class name, but offered a few suggestions.

if (!elem.shadowRoot) return elem;
const nestedSizedElem = elem.shadowRoot.querySelector('.vdiff-size');
if (!nestedSizedElem) return elem;
return findRealSize(nestedSizedElem);
Copy link
Member

Choose a reason for hiding this comment

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

Cool. So if an element doesn't define its size at all but instead that falls to something in its shadow root, it can just put the class on that element and it'll recurse -- nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah exactly! And in theory, this should "just work" for consumers wrapping a core component in that situation, because we would've had to already add it for everything that needs it for our own tests.

@@ -19,6 +19,13 @@ mocha.setup({
});
/* eslint-enable */

function findRealSize(elem) {
if (!elem.shadowRoot) return elem;
const nestedSizedElem = elem.shadowRoot.querySelector('.vdiff-size');
Copy link
Member

Choose a reason for hiding this comment

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

Alternate options for the CSS class: vdiff-rect-elem, vdiff-rect, vdiff-container

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had vdiff-rect and then I didn't love it, but I don't love vdiff-size either so switched back to that!

Copy link
Member

Choose a reason for hiding this comment

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

Throwing another option out there: vdiff-boundary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vdiff-area? vdiff-box? vdiff-frame? vdiff-actual?

Copy link
Contributor

Choose a reason for hiding this comment

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

vdiff-target?

@svanherk svanherk merged commit 4b9119c into main Aug 25, 2023
2 checks passed
@svanherk svanherk deleted the US156331_Add_ability_to_find_real_vdiff_size branch August 25, 2023 18:57
@ghost
Copy link

ghost commented Aug 25, 2023

🎉 This PR is included in version 0.30.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants