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

fix: position calculation for inline popups #1350

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Oct 4, 2024

All Submissions:

Changes proposed in this Pull Request:

https://app.asana.com/0/1200550061930446/1206767149727703/f

The logic used to calculate the position for the inline popups is not the same as applied to determining the position of the blocks.

When doing it for the block positions it's ignoring the HTML for inner blocks, causing blocks like lists to be miscalculated and inline prompts using percentage positioning to appear further down than they should.

This PR introduces a get_block_length method to normalize how they are calculated, also fixing the issue with lists.

How to test the changes in this Pull Request:

  1. While on the master branch, create a post with several list blocks, with 3/4 list items each, in the first half and paragraphs in the second half
  2. Create an inline prompt to appear at 50%
  3. Visit the post and confirm the prompt renders way below the 50% threshold
  4. Checkout this branch, refresh, and confirm it's now more within acceptable margins

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@miguelpeixe miguelpeixe self-assigned this Oct 4, 2024
@miguelpeixe miguelpeixe requested a review from a team as a code owner October 4, 2024 19:31
@miguelpeixe miguelpeixe marked this pull request as draft October 4, 2024 19:34
@miguelpeixe
Copy link
Member Author

This was already done and reverted in #855.

@dkoo, that's a tricky one that I know you've faced before. Publishers tweaked the prompt position until it was visually acceptable no matter the number, changing the logic will impact everyone regardless.

I'll keep this as a draft until we figure out how to deal with this.

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

It's still not perfect, but it's much better. For example, with the following content I'd expect the 50% block to be inserted between the second and third List blocks, but it's actually inserted between the first and second. But I think this is definitely an improvement over what we have, and the accuracy will probably get better with longer content as well.

<!-- wp:list -->
<ul class="wp-block-list"><!-- wp:list-item -->
<li>List item 1</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 1</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 2</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 3</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 4</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 5</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 6</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->

<!-- wp:list -->
<ul class="wp-block-list"><!-- wp:list-item -->
<li>List item 1</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 2</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 3</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 4</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 5</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 6</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->

<!-- wp:list -->
<ul class="wp-block-list"><!-- wp:list-item -->
<li>List item 1</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 2</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 3</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 4</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 5</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 6</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->

<!-- wp:list -->
<ul class="wp-block-list"><!-- wp:list-item -->
<li>List item 1</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 2</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 3</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 4</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 5</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>List item 6</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->

@dkoo
Copy link
Contributor

dkoo commented Oct 14, 2024

Looks like there's a failing test, too—does this indicate a regression in the "single group block + 0% prompt" scenario?

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

Successfully merging this pull request may close these issues.

2 participants