-
-
Notifications
You must be signed in to change notification settings - Fork 893
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 GridPlacement.columnEnd cannot exceed column count when using rowspans and colspans in a table #687
Conversation
…spans and colspans in a table
…into bugfix/column-row-offset
@ryan-berger so I guess the easiest way to explain it would be with an example: HTML:<table role="table">
<tbody>
<tr>
<td rowspan="2">
<p>CẤP HỌC</p>
</td>
<td colspan="2">
<p>HỌC KỲ II</p>
</td>
<td rowspan="2">
<p>NGÀY KẾT THÚC NĂM HỌC</p>
</td>
</tr>
<tr>
<td>
<p>Ngày bắt đầu HKII</p>
</td>
<td>
<p>Ngày kết thúc HKII</p>
</td>
</tr>
<tr>
<td>
<p>Mầm non</p>
</td>
<td>
<p>18/01/2021</p>
<p>(thứ Hai)</p>
</td>
<td>
<p>24/5/2021</p>
<p>(thứ Hai)</p>
</td>
<td>
<p>28/5/2021</p>
<p>(thứ Sáu)</p>
</td>
</tr>
</tbody>
</table> Rendered:
So the main fix here is with
As you can see, it added a "1" for the cells that had a rowspan > 1 in Row 1. Wherever there is a "1" in the list, the cell will be skipped adding there. So that would mean:
The issue was with row 3 - there isn't a position 4 to render in I don't think this should affect anything but @erickok is the better judge here as he made the original implementation. (Hopefully that made sense) |
I'll try to get my head on this tomorrow. |
Ok there was a but more to it than your analysis. First of all, there are only 4 columns (in this example html) so the 5 numbers in that list was obviously not correct. And then there were some other edge cases where the counting was wrong. I fixed those in a new PR. |
Also prevents crashes on edge cases where the rowspan or colspans are inconsistent/wrong
See #716 for the full fix |
Fixes #687 with a rowspan issue
Fixes #685
Don't know if this is the correct fix, I still don't fully understand what is going on with the table rendering, but it seems to do the trick.