feat: truncate component rework#4037
feat: truncate component rework#4037MaxFrank13 wants to merge 6 commits intoopenedx:release-23.xfrom
Conversation
✅ Deploy Preview for paragon-openedx-v23 ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
MaxFrank13
force-pushed
the
mfrank/truncate-component-refactor
branch
from
d13ac64 to
cca4d3a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-23.x #4037 +/- ##
================================================
+ Coverage 94.39% 94.52% +0.13%
================================================
Files 242 245 +3
Lines 4296 4330 +34
Branches 981 990 +9
================================================
+ Hits 4055 4093 +38
+ Misses 237 233 -4
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
On slack you mentioned needing to use inline styles. I wasn't able to completely get away from them but I was able to at least minimize it down to setting a css var diff --git a/src/Truncate/Truncate.tsx b/src/Truncate/Truncate.tsx
index fa421b68f6..ab9dd95753 100644
--- a/src/Truncate/Truncate.tsx
+++ b/src/Truncate/Truncate.tsx
@@ -9,11 +9,6 @@ interface TruncateProps {
}
function Truncate({ children, lines = 1 }: TruncateProps) {
- const style = {
- lineClamp: lines,
- WebkitLineClamp: lines,
- };
-
const initialText = Array.isArray(children) ? assembleStringFromChildrenArray(children) : String(children);
return (
@@ -21,7 +16,7 @@ function Truncate({ children, lines = 1 }: TruncateProps) {
title={initialText}
aria-label={initialText}
className="truncate-text"
- style={style}
+ style={{ "--truncate-prop-lines": lines } as React.CSSProperties}
data-testid="truncate-element"
>
{children}
diff --git a/src/Truncate/index.scss b/src/Truncate/index.scss
index a2f9fbde1f..418641008b 100644
--- a/src/Truncate/index.scss
+++ b/src/Truncate/index.scss
@@ -2,6 +2,6 @@
overflow: hidden;
display: -webkit-box;
-webkit-box-orient: vertical;
- -webkit-line-clamp: 1;
- line-clamp: 1;
+ -webkit-line-clamp: var(--truncate-prop-lines);
+ line-clamp: var(--truncate-prop-lines);
}As for the naming of the variable, this is definitely setting a new pattern so I'd love to hear opinions. My reasoning behind
|
Yes I think this makes sense and would be a clear way of indicating that this is a local variable and not a design token. W.r.t the style conventions, it also mentions that all CSS classes should be prefixed with |
|
@MaxFrank13 What's the status of this PR? |
Hey @bradenmacdonald! Just getting back from leave today. I believe this left off with us still considering what the best naming convention would be for the variable and class. If there are no objections to what Brian and I have mentioned in the earlier comments, then I can go ahead and make those changes. Curious if you have any thoughts on this? |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
@MaxFrank13 Great!
Yeah, I tend to agree that the CSS classes should have the pgn__ prefix, and the CSS var is better as just --truncate-prop-lines, since it's locally scoped to this component anyways.
src/Truncate/utils.ts
Outdated
| */ | ||
| export function assembleStringFromChildrenArray( | ||
| children: Array<React.ReactNode>, | ||
| elementsData: Array<ElementDataEntry> = [], |
There was a problem hiding this comment.
Nitpick: Modifying a passed array is a bit of an anti-pattern, unless the name of the function makes it obvious.
There was a problem hiding this comment.
Ah good call out! I've refactored so that we no longer use a passed array but instead initialize one within the function. In order for the recursion to work, this required returning the array as well as the string result. Because of this, the usage in Truncate.tsx had to be modified as well. I believe it's working exactly the same but would invite any criticisms there as well!
There was a problem hiding this comment.
LGTM! I haven't tested this though. You also have a bunch of lint warnings now.
There was a problem hiding this comment.
Ah! Something weird going on with VScode and the lint extension. Weird it only happens in Paragon repo for me. I can still run the npm run lint command though so I just did it that way 🤷
3 participants
Description
Corresponding issue: #3311
This refactors the
Truncatecomponent to use CSS properties instead of complex JS. The-webkit-line-clampproperty has support for most common browsers (source). Eventually,line-clampshould have support but that is not currently the case.This was discussed in Paragon Working Group (session notes).
Both
titleandaria-labelare used for accessibility in the same way it was used before. TheassembleStringFromChildrenArrayutility function has been moved to autils.tsfile and thus converted to TypeScript.The deprecated component as well as it's utils file still exist under the
Truncatedirectory. The deprecated component is still accessible atTruncate.Deprecated. Theutilsfile has been renamed toutils.deprecated. It didn't make sense to convert the entire utils file to TS when only the one utility function (assembleStringFromChildrenArray) was needed.The tests for the deprecated component are under
TruncateDeprecated.test.jsx.Deploy Preview
https://deploy-preview-4037--paragon-openedx-v23.netlify.app/components/truncate/
Merge Checklist
exampleapp?Post-merge Checklist