fix: removes TableCell wrapper

[ciduarte]

• removes TableCell wrapper from <td>

[coveralls]

Coverage increased (+0.004%) to 87.333% when pulling ac9c0b9 on ciduarte/aa-934 into d8dec97 on master.

[mikix]

I'm not fully aware of the problem here, but it might be good to name this like dataClassName or tdClassName or something?

Or pulling back, this distinction seems so minute - block level vs inline? Is that a real distinction we want to expose? Could we move the cellClassName up to the td level safely, to solve the same problem you are looking at? Or is that an API break?

[ciduarte]

I'm not sure what the original justification was for adding the cellClassName to the inline element vs the block element here, but I agree this feels a little redundant. Though, .pgn__data-table-cell-wrap has some styling that we may want to override (ex. .pgn__data-table-cell-wrap has a max-width and I found an edX repo that overrides it to none).

My use case for exposing the td element's className was to change its vertical alignment, which you can't access through the inline element.

I think given that there are use cases for both, it makes sense to expose both but update the names?

[mikix]

You may need someone more in tune with paragon than me.

Is moving .pgn__data-table-cell-wrap up to <td> and dropping the <span> altogether a bad idea?

[ciduarte]

I just tried to drop the span and update the styling and it didn't really go well, so I think that might be a bad idea

[adamstankiewicz]

I just tried to drop the span and update the styling and it didn't really go well, so I think that might be a bad idea

The code below seems to work locally without incorporating any breaking changes or the existing styles.

@ciduarte I think the styling issues you were running into might have been that .pgn__data-table-cell-wrap has display: block, so when it was placed on the td directly, the display: table-cell that should be on the td was overridden. Removing the display: block from the .pgn__data-table-cell-wrap styles seems to work well.

const TableCell = ({ getCellProps, render, column }) => (
  <td {...getCellProps({ className: classNames('pgn__data-table-cell-wrap', column.cellClassName) })}>
    {render('Cell')}
  </td>
);

.pgn__data-table-cell-wrap {
  max-width: 20vw;
  white-space: normal;
  overflow: hidden;
  text-overflow: ellipsis;
}

All the current uses of cellClassName should still work with this change, including the max-width: unset override. That override actually happens in two edX repos. One overrides the .pgn__data-table-cell-wrap class directly (anti-pattern), and the other passes a custom class name to cellClassName that handles the max-width: unset itself.

The other current uses of cellClassName will continue to work as is (e.g., float-right font-weight-bold small, mw-100).

[ciduarte]

@adamstankiewicz thanks for looking into this! I did try that yesterday, and didn't see any broken styling on the Paragon site. What I saw was specific to frontend-app-learning, and I couldn't get to the bottom of it so I assumed it would be an issue everywhere. Giving it a second look, I realized what was happening and this change feels safe to make!

For the curious, this change was impacting some cells that used float-right. Switching the cells to use text-right fixed the issue.

[adamstankiewicz]

[edx-semantic-release]

🎉 This PR is included in version 16.13.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀