Use overflowEllipsisDeclarations when lines=1 - #7293
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
| const spanClasses = { | ||
| 'd2l-link-content': true, | ||
| 'truncate': this.lines > 1, | ||
| 'truncate-one': this.lines === 1 | ||
| }; | ||
| const styles = { webkitLineClamp: this.lines || null }; | ||
| const styles = this.lines ? getOverflowDeclarations({ lines: this.lines }) : null; |
There was a problem hiding this comment.
Instead of handling 1 and >1 lines cases with separate classes, we can use the same it seemed simpler to just apply the styles inline. Even though the same call gets us the correct style now, the stylesheet doesn't update with property changes so it needs to either happen on render, or go back to separate classes
| export function getOverflowDeclarations({ textOverflow = '', lines = 0, lit = true } = {}) { | ||
| export function getOverflowDeclarations({ textOverflow = '', lines = 1, lit = true } = {}) { | ||
| if (!arguments.length) return overflowHiddenDeclarations; | ||
| if (arguments[0].lines === 1) return overflowEllipsisDeclarations; |
There was a problem hiding this comment.
Would this not ignore the textOverflow option?
There was a problem hiding this comment.
Also does just lines === 1 break? Or why do we need to use the args object?
There was a problem hiding this comment.
arguments because we want to check what was passed in, not what was defaulted. It will ignore textOverflow because if you're passing lines that means you inherently are expecting the standard ellipses overflow. It would do that now already since that's the only way line-clamp renders. It sets it but doesn't do anything now.
GZolla
left a comment
GZolla
left a comment
There was a problem hiding this comment.
Other than the missing }, LGTM
| if (this.disabled) { | ||
| e.stopPropagation(); | ||
| e.preventDefault(); | ||
| } |
There was a problem hiding this comment.
That suggestion was impossible to get to render correctly and it still broke anyway after 3 tries
3141fc1 to
3ec2de5
Compare
2 participants
PHNX-4815
In #7233 we adjusted the overflow clip margin to roughly follow the line-height to fix an issue with subsequent lines of text partially showing at the bottom of the clipped area. However, this only happens in Windows at mobile device widths (when we shrink the font-size and line-height) and when using
line-clampstyles. This is unlikely to occur in real-world usage already, but this PR changesline=1usages ofgetOverflowDeclarationsto always useoverflowEllipsisDeclarationsinstead ofline-clampwhich prevents this issue even further in the majority of uses.If it later becomes a problem in production we can try some other changes like adjusting the font-face styles in Windows etc.