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

US157225 - Enhance rect calculations #159

Merged
merged 6 commits into from
Sep 5, 2023

Conversation

svanherk
Copy link
Contributor

@svanherk svanherk commented Sep 5, 2023

Problem

The original vdiff-target calculation was a bit too limited for what we need. It didn't handle these two cases:

  1. You have two elements in your Shadow DOM that represent your full size. For example, count-badge has a div and a d2l-tooltip. With no vdiff-target set it would only include the div in the screenshot, and if vdiff-target is added to tooltip it would only include the tooltip.
  2. You have a vdiff test with multiple components you want included, but one or more is absolute or fixed positioned. For example, a vdiff of:
    <d2l-button id="button">Always visible</d2l-button><d2l-tooltip for="button" force-show >Visible</d2l-tooltip>

Solution

The code below now:

  1. Accepts multiple vdiff-targets being set, and creates the largest rect needed to accommodate all of them.
  2. Allows someone to add the vdiff-include class to fixture templates to specify elements that should also be included in the above calculation.

Example

Let's say I want a vdiff test of this:

<div style="display: inline-block;">
	<d2l-dialog-confirm id="confirm" title-text="Title" text="Are you sure?" opened>
		<d2l-button slot="footer" primary>Yes</d2l-button>
		<d2l-button slot="footer" id="cancel">No</d2l-button>
	</d2l-dialog-confirm>
	<d2l-button id="tooltip-force-show">Always visible</d2l-button>
	<d2l-tooltip for="tooltip-force-show" force-show >
		Always visible message
	</d2l-tooltip>
	<d2l-count-badge style="position: absolute; right: 80px;" has-tooltip number="10" text="10 new notifications"></d2l-count-badge>
</div>

As is, that would look like this:
image
We could try to make the div some arbitrary size to capture everything, but that's no good. Instead, we can add vdiff-include to the elements not automatically captured because they have position: absolute or position: fixed:

<div style="display: inline-block;">
	<d2l-dialog-confirm class="vdiff-include" id="confirm" title-text="Title" text="Are you sure?" opened>
		<d2l-button slot="footer" primary>Yes</d2l-button>
		<d2l-button slot="footer" id="cancel">No</d2l-button>
	</d2l-dialog-confirm>
	<d2l-button id="tooltip-force-show">Always visible</d2l-button>
	<d2l-tooltip class="vdiff-include" for="tooltip-force-show" force-show >
		Always visible message
	</d2l-tooltip>
	<d2l-count-badge class="vdiff-include" style="position: absolute; right: 80px;" has-tooltip number="10" text="10 new notifications"></d2l-count-badge>
</div>

Now we get:
image
Much better! But as you can see, count-badge is still getting cut off because it's not incorporating the tooltip size. If we set vdiff-target on the div here AND on the d2l-tooltip here we get:
image
Magic!

@svanherk svanherk requested a review from a team as a code owner September 5, 2023 14:37
github-actions bot and others added 3 commits September 5, 2023 10:43
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 really straightforward to me, nice job!

@svanherk svanherk merged commit a2720d0 into main Sep 5, 2023
3 checks passed
@svanherk svanherk deleted the US157225_Enhance_rect_calculations branch September 5, 2023 17:12
@ghost
Copy link

ghost commented Sep 5, 2023

🎉 This PR is included in version 0.32.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants