progress table layout cleanup #39091
Conversation
…d content view. fixes header alignment issue in detail view.
…visible unplugged bubble to determine the width of the unplugged header icon.
cforkish
force-pushed
the
cforkish/table-layout-cleanup
branch
from
b5fe0e3
to
106e827
Compare
| @@ -166,7 +168,8 @@ export default class ProgressTableContentView extends React.Component { | |||
| rowKey={'id'} | |||
| onScroll={this.props.onScroll} | |||
| style={{ | |||
| overflow: 'auto', | |||
| overflowX: 'scroll', | |||
There was a problem hiding this comment.
i tried just setting this to scroll for both to avoid the need for any gutter calculations, but deemed the visual benefit of only showing it conditionally to be well worth the cost.
There was a problem hiding this comment.
In case this was confusing to anyone else, this comment applies to L171-172 and explains why overflowY is not set to 'scroll'
| @@ -103,9 +103,11 @@ export default class ProgressTableContentView extends React.Component { | |||
|
|
|||
| columnWidthStyle(index) { | |||
There was a problem hiding this comment.
Might be worth adding a comment somewhere around here describing when we expect columnWidths to be specified and when not and the overall behavior associated with each case.
| @@ -166,7 +168,8 @@ export default class ProgressTableContentView extends React.Component { | |||
| rowKey={'id'} | |||
| onScroll={this.props.onScroll} | |||
| style={{ | |||
| overflow: 'auto', | |||
| overflowX: 'scroll', | |||
There was a problem hiding this comment.
In case this was confusing to anyone else, this comment applies to L171-172 and explains why overflowY is not set to 'scroll'
| // The width of our unplugged bubble depends on the localization of the text, | ||
| // so to allow that to be determined at render time, we don't set an explicit | ||
| // width for unplugged bubbles and instead render an invisible one behind the | ||
| // icon to determine its width. |
There was a problem hiding this comment.
I could use some clarification here, are we rendering the hidden progress bubble here because the header is determining the width of the column?
There was a problem hiding this comment.
more specifically, we're adding the hidden progress bubble here so that the intrinsic content width of the unplugged icon will match that of the bubble, regardless of context.
hopefully the comment i just added to ProgressTableContentView.columnWidthStyle will clarify why this is necessary -- the content width of the ProgressTableLevelBubbles determine the width of the body columns, but for our header columns to have the same width without it being provided explicitly, they need to have the same content width as the body columns. so in our case, this is determining the width of the header columns, but specifically not of the body columns (ProgressTableLessonNumber sizes itself based on its container, so does not contribute to the determination of the header column width). lmk if that's still unclear!
we discovered a few non-blocking bugs in our table layout during final testing, which this PR resolves.
before
after
unplugged widths
we were using a hacky method to calculate the width of a localized unplugged bubble by rendering it in the dom, measuring it, then removing it. however, i discovered right after we shipped that chrome was producing the wrong value in this calculation. after some investigation it turned out that our font wasn't fully loaded by the time the calculation was run, resulting in a smaller pixel width.
after trying a few different ways to solve this width calculation problem, i decided to revisit the underlying need for explicit widths in the first place. in short, while there were several motivating factors, the only actual need was for calculating the width of the table to conditionally render a gutter at the bottom of the student list when the content view is displaying a scrollbar. it had never occurred to me before that a much simpler (though slightly less visually appealing) solution was to just have both parts of the table always show a gutter to avoid the need to know if the content view is showing a scrollbar.
making this change allowed for a lot of good code simplification. however, due to how reactabular handles sticky headers and body virtualization, our table headers are unable to automatically size themselves based on cell content in the body, which means the headers either need to know the exact width of their body columns, or they need to render something of the same size. the way i handled this before running into the scrollbar-gutters-need-table-width issue was to simply render an invisible dummy detail cell in the header to define its size. this is obviously also hacky, but imo much less so than the previous dom measurement solution. and since the unplugged bubble is the only bubble with a dynamic size, i'm only using the hack for those icons.
tl;dr -- our detail view content table is now sizing its columns based on content size rather than explicit width. in the headers, it gets that size from the ProgressTableLevelIcon. in the body, it gets the same size from the ProgressTableDetailCell. summary view content table is still using explicit width constraints to avoid some off-by-one rounding errors between header and body cells that i was seeing during development.