Truncate: Convert component to TypeScript #41697
Conversation
walbo
added
[Type] Enhancement
|
Size Change: -3 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
| const sx = { | ||
| numberOfLines: css` | ||
| -webkit-box-orient: vertical; | ||
| -webkit-line-clamp: ${ numberOfLines }; | ||
| display: -webkit-box; | ||
| overflow: hidden; | ||
| `, | ||
| }; |
There was a problem hiding this comment.
I'd be curious to know if there was an original reason for the way code was written before — anything comes to mind?
There was a problem hiding this comment.
Not sure the original reason. By adding it directly into the object we don't have to add types to the object since that will be done automatically.
No strong options on this so can be changed back.
There was a problem hiding this comment.
I'm gonna guess it's a style that was adopted for adding conditional properties like this:
gutenberg/packages/components/src/elevation/hook.js
Lines 79 to 85 in f58a3c0
gutenberg/packages/components/src/text/hook.js
Lines 106 to 113 in f58a3c0
Which... I'm not particularly fond of because I tend to find functional programming more readable. But that's just a personal thing.
And TIL that sx is a thing in MUI that facilitates adding responsive styles. So I think that explains the intent of that variable's name and coding style.
I don't think we need to overthink it though — we can simplify where we can, and that includes maybe renaming sx to something that is more widely understood, e.g. responsiveStyles. Or in this case, removing the sx wrapper object entirely.
Also I'm still not that experienced with CSS-in-JS architectures, so I don't know why certain styles are written in hooks.ts and not styles.ts 🙃
There was a problem hiding this comment.
I don't think we need to overthink it though
Absolutely! I just asked because I've seen this pattern a few times already and wasn't aware of the reasons behind — totally cool for me to simplify code / rename variables if necessary.
Also I'm still not that experienced with CSS-in-JS architectures, so I don't know why certain styles are written in hooks.ts and not styles.ts 🙃
I also wondered why, and I don't think I have a good answer to it yet. I'd be in favour of picking one approach and just sticking with it
There was a problem hiding this comment.
Renamed the variable for now. It's a bit strange that some CSS are in styles and some in hooks. Makes it hard to find the style.
Not sure if I should do anything about it in this PR?
There was a problem hiding this comment.
As a soft guideline, let's try to keep styles in the styles file then. For existing code, we could try moving stuff over if it feels appropriate for the scope of the PR, e.g. if it's a small styles-related PR. Let's see how that feels.
| const sx = { | ||
| numberOfLines: css` | ||
| -webkit-box-orient: vertical; | ||
| -webkit-line-clamp: ${ numberOfLines }; | ||
| display: -webkit-box; | ||
| overflow: hidden; | ||
| `, | ||
| }; |
There was a problem hiding this comment.
I'm gonna guess it's a style that was adopted for adding conditional properties like this:
gutenberg/packages/components/src/elevation/hook.js
Lines 79 to 85 in f58a3c0
gutenberg/packages/components/src/text/hook.js
Lines 106 to 113 in f58a3c0
Which... I'm not particularly fond of because I tend to find functional programming more readable. But that's just a personal thing.
And TIL that sx is a thing in MUI that facilitates adding responsive styles. So I think that explains the intent of that variable's name and coding style.
I don't think we need to overthink it though — we can simplify where we can, and that includes maybe renaming sx to something that is more widely understood, e.g. responsiveStyles. Or in this case, removing the sx wrapper object entirely.
Also I'm still not that experienced with CSS-in-JS architectures, so I don't know why certain styles are written in hooks.ts and not styles.ts 🙃
| | 'middle' | ||
| | 'none'; | ||
|
|
||
| export type TruncateProps = { | ||
| /** | ||
| * The ellipsis string when `truncate` is set. |
There was a problem hiding this comment.
As with the limit prop, the truncate this description is referencing is the truncate prop on components that are based on <Truncate> (e.g. <Text>, <Heading>, maybe more). So it's nonsensical as a description in <Truncate> itself.
In light of the behavior quirk that Marco found, and to avoid having to override these descriptions for the Truncate-based components, maybe we can say "The ellipsis string when truncating the text by a limit value"?
@ciampo Aside: Maybe this is another instance where grouping the pass-through props together is a good idea. Like for the Heading props doc, we could've just had one prop truncateProps with the description "Settings for truncating the text. See the Truncate documentation for details.".
There was a problem hiding this comment.
maybe we can say "The ellipsis string when truncating the text by a limit value"?
Sounds good to me, maybe to be clear we can say "the limit prop's value" ?
Maybe this is another instance where grouping the pass-through props together is a good idea. Like for the Heading props doc, we could've just had one prop truncateProps with the description "Settings for truncating the text. See the Truncate documentation for details.".
This is definitely worth considering, yes! I'd like to take a wider look at the package, though, and understand if this is a good pattern that we can adopt throughout the codebase (or if, for example, there are better alternatives). Ultimately, I'd love it if the components had consistent APIs for composition
| * Clamps the text content to the specified `numberOfLines`, adding the | ||
| * `ellipsis` at the end. | ||
| * | ||
| * @default 0 | ||
| */ | ||
| numberOfLines?: number; |
There was a problem hiding this comment.
I also realised that this type is not a great fit for numberOfLines: according to the spec, only positive integer numbers make sense (apart from other string keywords).
Even a default of 0 is non-sensical
There was a problem hiding this comment.
TIL. Good to know the spec there.
| shouldTruncate && ! numberOfLines && styles.Truncate, | ||
| shouldTruncate && !! numberOfLines && sx.numberOfLines, | ||
| shouldTruncate && !! numberOfLines && truncateLines, |
There was a problem hiding this comment.
Update: Sorry I almost forgot this was a TS migration PR. No runtime changes! 😂
Nit: I find this 👇 simpler to read, might just be me though.
shouldTruncate &&
( numberOfLines ? truncateLines : styles.Truncate ),| * Clamps the text content to the specified `numberOfLines`, adding the | ||
| * `ellipsis` at the end. | ||
| * | ||
| * @default 0 | ||
| */ | ||
| numberOfLines?: number; |
There was a problem hiding this comment.
TIL. Good to know the spec there.
What?
Converts the
Truncatecomponent to TypeScript.Why?
Part of the @wordpress/components's TypeScript migration (#35744).
How?
Refactors the current
Truncateto TypeScript.Testing Instructions
Truncatecontinues to function as expected