TreeNode: set max-width #3370
TreeNode: set max-width #3370
Conversation
|
I also see that, at larger font sizes at widths exceeding the 560px breakpoint that I was targeting, we can also get some cutoff. I can try to fix that as part of this issue if you want. 
|
|
Sure! You can address the other case in this PR unless it ends up being a bigger change. It would be great to get a puppeteer snapshot test covering this at different screen widths. Setting puppeteer's window width can be tricky sometimes, but give it a try. |
I added some tests, and they showing that my approach has problems with thought annotations. Putting the |
|
Okay, sounds good. At the end we'll need to reduce this down to the fewest tests that are needed cover the behavior, since snapshot tests are expensive and we don't want to slow down the test suite unnecessarily. |
I figured out that the margin on |
| // Add a negative marginRight equal to translateX to ensure the thought takes up the full width. | ||
| const marginRight = `${-indent + (isTouch ? 2 : -1)}em` |
There was a problem hiding this comment.
We actually prefer inlining this since it is only used in one place and it is easier to understand when co-located with the element that uses it. If the code becomes too verbose, it's better to split into smaller components than split the CSS away from the DOM.
| {treeThoughtsPositioned.map((thought, index) => ( | ||
| <TreeNode | ||
| {...thought} | ||
| style={{ |
There was a problem hiding this comment.
An object literal will be a new reference every time, even if maxWidth hasn't changed. While TreeNode is not currently memoized, I think it's generally a good practice to preserve referential integrity of props. Therefore, I recommend using useMemo to create a stable style object that can be passed to TreeNode.
There was a problem hiding this comment.
This is taking place inside of treeThoughtsPositioned.map, so my options for memoizing it are to:
- create a new component
- memoize the entire
treeThoughtsPositionedobject
I think option 2 makes sense to me, but I figured I should check in case that introduces too much risk.
There was a problem hiding this comment.
Oof, that's a lot of restructuring to get this memoized. Yes, I think option 2 makes the most sense.
An alternative is to pass indent as a prop and allow TreeNode to calculate and memoize maxWidth. That would be a bit of a lighter touch.
|
@BayuAri Ready for testing! |
BayuAri
left a comment
BayuAri
left a comment
There was a problem hiding this comment.
Confirmed, the layout issue is no longer reproducible.
Text is not cut off before wrapped.
Video:
Text.is.not.cut.off.at.small.screen.before.wrapped.mov
3 participants
Fixes #3353
I added a
max-widthtoTreeNodein order to stop it from overrunning the viewport width when thetransformapplied to indented thoughts exceeds the right padding. In order to do this, I needed to updated bulletWidth in LayoutTree in order to calculate the width of the bullet in the same way that it is calculated in Bullet. This might have unforeseen consequences, and I could duplicate that calculation insideTreeNodeinstead.