Components: Add truncate #28176
Components: Add truncate #28176
Conversation
sarayourfriend
requested review from
youknowriad,
gziolo,
ItsJonQ and
jorgefilipecosta
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +6 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
packages/components/src/truncate/test/__snapshots__/truncate.js.snap
Outdated
Show resolved
Hide resolved
| return ( | ||
| word.slice( 0, frontLength ) + | ||
| truncateStr + | ||
| word.slice( wordLength - backLength ) |
There was a problem hiding this comment.
I'm pretty sure this kind of truncating using substrings is language specific. How does this component behave in other languages? cc @swissspidy
There was a problem hiding this comment.
Some tests with strings in other languages would be good.
So if I understand this right, truncateMiddle( 'Hello', 1, 1, '...') will return H...o.
Sounds OK when using Latin alphabet, but with strings like こんにちは ("Kon'nichiwa", "Hello" in Japanese) this will result in こ...は, which probably is less understandable.
Not sure how it would work with Arabic, but you'd know that best :-)
Worst case we'd need some words/characters logic like the word count package has
There was a problem hiding this comment.
Interesting 🤔 ! if there's a library folks are aware of that could elegantly handle this, I think it would be a worth while addition :D.
For context, So far, we've only used it for Truncating... handling.
I based this component on something I've built in the past. Also heavily drawing inspiration from how Text and truncation is handled in React Native:
There was a problem hiding this comment.
Adding more thoughts!
Taking a step back, for the use-cases of user interfaces, realistically the only situations were we'd need to truncateMiddle would be for long file names.
For example:
thequickbrownfoxjumpsoverthelazydog.jpg
maybe becomes
theq...ydog.jpg
This is common for attachment related UIs or slug/tags.
In that case, it doesn't necessarily matter if the word/letters is linguistically understandable.
As long as there's enough to identify either the extension (e.g. .jpg) or the file / keyword / slug :)
There was a problem hiding this comment.
Excuse my idiot questions:
- if middle is not the important use-case for this component, should we include it?
- It seems this code has some logic to split words... I believe we may already have some similar logic in
@wordpress/wordcountthat is very similar and that works across languages, If we keep this behavior, do you think there's a way to kind of consolidate that logic somehow?
There was a problem hiding this comment.
I think we should include middle as long as the use case that @ItsJonQ brought up is one that we'll need available at any time (or if block developers would find it useful for file names).
We don't do any word counting in this component, just line counting. Was there a specific place @youknowriad that you would see us using @wordpress/wordcount for this component?
There was a problem hiding this comment.
I guess I misunderstood the code but I thought there was some smart behavior to try to split between "words" or something like that in which case, it would be a similar algorithm to word count to figure out where are the words.
There was a problem hiding this comment.
There’s no such logic, as far as I can see. @ItsJonQ maybe you can chime in here to verify.
There was a problem hiding this comment.
I don't believe there's logic to split words - just splitting characters.
| */ | ||
| import { BaseView } from '@wp-g2/styles'; | ||
|
|
||
| const View = BaseView; |
There was a problem hiding this comment.
How is this component different from the "View" primitives we have? Should it be a wrapper around it, or does it replace it?
There was a problem hiding this comment.
View from primitives is a barebones (web) div or a View from React Native.
The BaseView from G2 is the entire foundation of the library's style system, shipped as a single base element:
https://github.com/ItsJonQ/g2/blob/master/packages/create-styles/src/create-style-system/create-core-element.js
Should it be a wrapper around it, or does it replace it?
For the web implementation, IMO it should replace the primitives/view that we currently have (once we're ready).
If creating a components/view component is a concern at this stage, we can refactor things on the @wp-g2/* side so it would instead be:
import { View } from '@wp-g2/styles';Rather than:
import { View } from '../view';Let us know 🙌
There was a problem hiding this comment.
I don't know yet :) I think it's fine to keep it. but maybe for the moment don't expose outside the components package especially since it has the css and as props which we might not want to expose.
Curious what other folks think though.
There was a problem hiding this comment.
I don't think we should ever expose View in the exports. I don't see any reason to expose it to the outside world, I can't think of the use case for it unless someone was building their own component library on top of G2, but then they should start with create-styles rather than styles/View.
There are other things too, createComponent shouldn't be exported either. If we don't want the "guts" of G2 to be exposed then we shouldn't export these things.
|
@saramarcondes I checked this out locally on my computer. Things are building okay. I'm not too sure why the tests are failing at dependencies/type checking :( |
|
It might be something with the lock file so rebasing with the main branch might indeed resolve it 👍 |
| @@ -0,0 +1,101 @@ | |||
| --- | |||
There was a problem hiding this comment.
Does it require any additional tooling to regenerate README file?
There was a problem hiding this comment.
It seems like there is some different tools used to generate that README file. I removed the extra markup and now it looks like the rest of component's README's.
|
@saramarcondes I just merged in the FontSizeControl PR! I'm going to consolidate that with this one :) |
gziolo
added
[Feature] Component System
|
@gziolo @youknowriad Is there anything blocking this PR ATM? Looks like everything is green and all requests have been addressed. |
sarayourfriend
force-pushed
the
add/truncate
branch
from
d6aa72b
to
4bc5092
Compare
|
Hmm! Not sure why the E2E tests are unhappy. Rerunning them :) |
|
I think this latest E2E failure is "okay", as we're seeing similar failures on the latest commits. |
|
I've been suggested that possibly this PR had a small typo resulting in #28510. |
Use to truncate text contents to a particular length or number of lines. Available as of Gutenberg 9.9 https://make.wordpress.org/core/2021/02/05/whats-new-in-gutenberg-9-9-5-february/ WordPress/gutenberg#28176
Description
Migrates the
Truncatecomponent from G2. This is a dependency ofText. Also brings in the simple and smallViewand less simple but still smallcreateComponent.ViewandcreateComponentare unexported (just internal).Truncateis exported as__experimentalTruncate.How has this been tested?
Unit tests pass. These components are unused outside of Storybook (so check there too).
Types of changes
New features
Checklist: