Conversation
jiayingxu
changed the title
jiayingxu
force-pushed
the
jiayingxu/table-sticky-footer-offset-fix
branch
from
3716d2d to
930440e
Compare
jiayingxu
requested review from
bantic,
kpfefferle,
mixonic,
mmun,
shaun-addepar and
twokul
bantic
force-pushed
the
jiayingxu/table-sticky-footer-offset-fix
branch
from
5fc2687 to
930440e
Compare
jiayingxu
force-pushed
the
jiayingxu/table-sticky-footer-offset-fix
branch
from
930440e to
63b5b51
Compare
bantic
approved these changes
Nov 9, 2020
Contributor
bantic
left a comment
bantic
left a comment
There was a problem hiding this comment.
Good catch, fix looks good! 👏
Is there a way to confirm that the table header/footer rows do indeed span multiple lines, in the tests? Is it just the length of the prefix text that makes them take multiple lines? If the table were wider, would they cease being multiline rows?
Is it possible to, for instance, amend the tests with an assertion that the rows are indeed inequal heights?
Provisional ✔️ pending addressing ^.
bantic
reviewed
Nov 9, 2020
Contributor
Author
|
@bantic, good point about verifying that the table header/footer rows span multiple lines. Currently, it is dependent on the length of the prefix text and the width of the table. I've added a pre-condition assertion to verify that the header/footer rows have unequal heights. |
jiayingxu
force-pushed
the
jiayingxu/table-sticky-footer-offset-fix
branch
from
e88b7ad to
680f71d
Compare
jiayingxu
force-pushed
the
jiayingxu/table-sticky-footer-offset-fix
branch
from
680f71d to
50d74f4
Compare
delete-merged-branch
bot
deleted the
jiayingxu/table-sticky-footer-offset-fix
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
TableStickyPolyfillis used to position header and footer rows. The individualthandtdoffset positions are calculated for the footer rows so they remain “sticky”. There is a bug in the logic for calculating the bottom offset for table footer rows where therowswere being iterated backwards while theheightswere iterated forwards. This bug manifests when table footer rows have varying heights and the list of heights is not a palindrome.As shown in the image below, the table has 7 footer rows where the height of the 1st footer row is 74px and the height of all other footer rows is 26px. The bottom offset of the 6th row takes into account the height of the 1st row and not the 7th row due to the loop iteration direction bug.
The list
heights: [74, 26, 26, 26, 26, 26, 26]is errantly being read in reverse order asheights: [26, 26, 26, 26, 26, 26, 74].The ember-twiddle demonstrates the issue here.