[RCN-685]: Table cell wrap #4018
Conversation
|
Visit https://backpack.github.io/storybook-prs/4018 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4018 to see this build running in a browser. |
| ); | ||
|
|
||
| // Demonstrates opt-in wrapping via wrap={true}. Long text will wrap onto multiple lines. | ||
| const WrapExample = () => ( |
There was a problem hiding this comment.
You should add another test with an extremely long, unbroken word like Llanfairpwllgwyngyllgogerychwyrndrobwllllantysiliogogogoch as that is going to be an issue in German, Finnish and Hungarian (plus Welsh if we ever support it).
| } | ||
|
|
||
| // Wrapping is opt-in; default is single-line via inherited nowrap styling. | ||
| &--wrap { |
There was a problem hiding this comment.
Nitpick but I'd be more specific about which type of wrap this enforces - word break and break all aren't the same.
| const BpkTableCell = ({children, className = null, wrap = false, ...rest}) => { | ||
| const classNames = getClassName( | ||
| 'bpk-table__cell', | ||
| wrap && 'bpk-table__cell--wrap', |
There was a problem hiding this comment.
Sorry for the vague comment but this doesn't feel like the right place to be doing this check.I'd personally write something that builds a array of classnames up and spread that into this.
|
|
||
| `wrap` (boolean, default `false`) | ||
|
|
||
| Opt-in multi-line wrapping for cell content. By default (prop omitted or `false`) table cells render on a single line (nowrap). Set `wrap={true}` to apply the `bpk-table__cell--wrap` modifier class which sets `white-space: normal;` and allows long text to wrap onto multiple lines. Available on both `BpkTableCell` and `BpkTableHeadCell`. |
There was a problem hiding this comment.
As with above-
"Wrap" can mean a lot more than just word-wrap: break-word; and so this property name should be far more explicit in what it's doing.
|
Visit https://backpack.github.io/storybook-prs/4018 to see this build running in a browser. |
| <BpkTable> | ||
| <BpkTableHead> | ||
| <BpkTableRow> | ||
| <BpkTableHeadCell wrap>Extremely Long Header Title That Will Wrap When Space Is Constrained</BpkTableHeadCell> |
There was a problem hiding this comment.
Thank you for adding another example for the new feature. 👍
Could you add a URL content to one of these table cells to check if this satisfies the edge case you mentioned in our Slack conversation? THX.
| BpkTableHeadCell.propTypes = { | ||
| children: PropTypes.node.isRequired, | ||
| className: PropTypes.string, | ||
| wrap: PropTypes.bool, |
There was a problem hiding this comment.
It may be a little confusing for other consumers. Can we rename it to a more readable name like allowWrap or wrapText, for clarity?
| </thead> | ||
| </table>, | ||
| ); | ||
| expect(asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
Currently, we do not encourage contributors to use toMatchSnapshot. Could you use the jest expect DOM element instead?
| expect(asFragment()).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| it('should render correctly with wrap enabled', () => { |
There was a problem hiding this comment.
I'm glad to see you added a new unit test for this prop, as mentioned in the comment r2467784300. I was considering the test coverage and was wondering if it would be possible to add a unit test for an edge case, such as a long text or link.
| </tbody> | ||
| </table>, | ||
| ); | ||
| expect(asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
We currently do not encourage contributors to use toMatchSnapshot. Could you use the Jest expect function for DOM elements and help modify other use cases in this file to duplicate snapshot files? 🙇
Remember to include the following changes:
[Clover-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.md