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

Equal heights: Improve row detection (exclude hidden elements) #9639

Merged

Conversation

EricDunsworth
Copy link
Member

The plugin detects new "virtual" rows by comparing the vertical offsets of each element designated to be equalized. New rows are established when back-to-back elements are detected as having different offsets.

#9589 fixed a bug that prevented it from working properly on deeply-nested elements using the eqht-trgt class. How? By changing how offsets were detected into a calculation representing the viewport's vertical scroll position + each element's top offset relative to the viewport.

The new calculation logic worked fine for visible elements, but not on hidden (display: none;) ones. Why? Because hidden elements don't have a top viewport offset. So inconsistent offsets were getting used when comparing visible elements against hidden ones.

End result was that if a visible element was followed by a hidden one, the plugin would end the current row and establish a new one... even if a subsequent visible element was still on the old row. That caused mismatched equalization and potentially-broken float layouts.

This fixes it by modifying the row detection logic to exclude hidden elements (i.e. anything with an offsetHeight of 0).

Partially addresses #9607.

The plugin detects new "virtual" rows by comparing the vertical offsets of each element designated to be equalized. New rows are established when back-to-back elements are detected as having different offsets.

wet-boew#9589 fixed a bug that prevented it from working properly on deeply-nested elements using the eqht-trgt class. How? By changing how offsets were detected into a calculation representing the viewport's vertical scroll position + each element's top offset relative to the viewport.

The new calculation logic worked fine for visible elements, but not on hidden (display: none;) ones. Why? Because hidden elements don't have a top viewport offset. So inconsistent offsets were getting used when comparing visible elements against hidden ones.

End result was that if a visible element was followed by a hidden one, the plugin would end the current row and establish a new one... even if a subsequent visible element was still on the old row. That caused mismatched equalization and potentially-broken float layouts.

This fixes it by modifying the row detection logic to exclude hidden elements (i.e. anything with an offsetHeight of 0).

Partially addresses wet-boew#9607.
@EricDunsworth
Copy link
Member Author

I'll try to post a simple code sample at some point that can be used to verify the fix.

My own testing was done on a local copy of #9607 (comment)'s page that I tried simplifying as much as possible. But it was still kind of enormous and relied on custom JS. So I might try to simplify things even further to make it easier to review.

@EricDunsworth
Copy link
Member Author

@Ricokola Here's a code sample that can be used to verify this PR's fix:

  • Contains 7 grids spread across 2 rows.
  • Grid 2 is hidden (via the hidden attribute).
  • Before this PR, grid 2's presence would've caused the equal heights plugin to think a new row was starting - resulting in grid 1 not getting equalized correctly. The plugin treated grid 1, 2 and 3-4 as three separate rows.
  • After this PR, the plugin ignores grid 2 since it has no height. So grids 1 and 3-4 get treated as the same row.

Try throwing the code sample into a test page's content area and comparing how it looks before/after this PR.

HTML code sample:

<h1 id="wb-cont" property="name">Example of a hidden grid throwing-off equalization</h1>
<div class="row wb-eqht">
	<div class="col-sm-4">
		<section class="panel panel-default hght-inhrt">
			<div class="panel-heading">
				<h3 class="panel-title">Grid 1</h3>
			</div>
			<div class="panel-body">
				<p>Line 1</p>
			</div>
		</section>
	</div>
	<div class="col-sm-4" hidden>
		<section class="panel panel-default hght-inhrt">
			<div class="panel-heading">
				<h3 class="panel-title">Grid 2 (hidden)</h3>
			</div>
			<div class="panel-body">
				<p>Line 1<br>Line 2</p>
			</div>
		</section>
	</div>
	<div class="col-sm-4">
		<section class="panel panel-default hght-inhrt">
			<div class="panel-heading">
				<h3 class="panel-title">Grid 3</h3>
			</div>
			<div class="panel-body">
				<p>Line 1<br>Line 2<br>Line 3</p>
			</div>
		</section>
	</div>
	<div class="col-sm-4">
		<section class="panel panel-default hght-inhrt">
			<div class="panel-heading">
				<h3 class="panel-title">Grid 4</h3>
			</div>
			<div class="panel-body">
				<p>Line 1<br>Line 2<br>Line 3<br>Line 4<br>Line 5</p>
			</div>
		</section>
	</div>
	<div class="col-sm-4">
		<section class="panel panel-default hght-inhrt">
			<div class="panel-heading">
				<h3 class="panel-title">Grid 5</h3>
			</div>
			<div class="panel-body">
				<p>Line 1</p>
			</div>
		</section>
	</div>
	<div class="col-sm-4">
		<section class="panel panel-default hght-inhrt">
			<div class="panel-heading">
				<h3 class="panel-title">Grid 6</h3>
			</div>
			<div class="panel-body">
				<p>Line 1<br>Line 2<br>Line 3</p>
			</div>
		</section>
	</div>
	<div class="col-sm-4">
		<section class="panel panel-default hght-inhrt">
			<div class="panel-heading">
				<h3 class="panel-title">Grid 7</h3>
			</div>
			<div class="panel-body">
				<p>Line 1<br>Line 2<br>Line 3<br>Line 4<br>Line 5</p>
			</div>
		</section>
	</div>
</div>

@duboisp
Copy link
Member

duboisp commented Aug 7, 2023

Approved upon successful testing and after the test case example is added to our code base separately of our current working example.

@duboisp duboisp force-pushed the v4.0-equalheight-row-exclude-hidden-elm branch from 0ec4023 to df401a8 Compare August 15, 2023 18:24
@duboisp duboisp force-pushed the v4.0-equalheight-row-exclude-hidden-elm branch from df401a8 to 00b0b64 Compare August 15, 2023 18:25
Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Tested and reviewed locally, work as expected.

I added the special test case example as documented in the PR comments

@duboisp duboisp merged commit 8fd8e09 into wet-boew:master Aug 15, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants