[table] fix wrapText on EditableCell component #2701
Conversation
|
Thanks for your interest in palantir/blueprint, @face! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
|
Or click the failing build link, it has a nice report at the top |
|
@giladgray thanks, lint fixed |
| expect(elem.find(`.${Classes.TABLE_NO_WRAP_TEXT} div`).length).to.equal(2); | ||
| }); | ||
|
|
||
| it("wraps text when wrapsText is true", () => { |
| it("wraps text when wrapsText is true", () => { | ||
| const elem = mount(<EditableCell wrapText={true} />); | ||
|
|
||
| expect(elem.find(`.${Classes.TABLE_NO_WRAP_TEXT} div`).length).to.equal(0); |
There was a problem hiding this comment.
This is an inscrutable selector. Why the child div? Is it necessary? Can you come up with a more legible assertion? Maybe just the length of children on TABLE_NO_WRAP_TEXT?
There was a problem hiding this comment.
Thank you @giladgray. You are correct, the div is not necessary. Added children().length. Is this what you wanted?
| it("defaults to no wrapText", () => { | ||
| const elem = mount(<EditableCell />); | ||
|
|
||
| expect(elem.find(`.${Classes.TABLE_NO_WRAP_TEXT}`).children().length).to.equal(1); |
There was a problem hiding this comment.
actually this doesn't do what you think it does: https://github.com/airbnb/enzyme/blob/master/docs/guides/migration-from-2-to-3.md#children-now-has-slightly-different-meaning.
i think this test can be simply expect(elem.find(.${Classes.TABLE_NO_WRAP_TEXT}).exists()).to.be.true; and the other one .to.be.false
There was a problem hiding this comment.
the .exists() check is very intuitive: no wrap text means the NO_WRAP_TEXT element exists.
There was a problem hiding this comment.
@face can I get a push from you real quick to resolve this?
giladgray
changed the title
giladgray
changed the title
|
merging, will fix comments myself |
|
thanks @giladgray, sorry I dropped the ball and didn't see your request. I got pulled onto another project and am just now back on the one using bluprintjs. I need to figure out a better way to manage my github alerts as I would have addressed this had I not missed your request. thanks again for finalizing and merging the fix. |
Fixes #2700
Checklist
Changes proposed in this pull request:
Fixes bug where wrapText={true} has doesn't work for
Reviewers should focus on:
fix and tests
Screenshot