Implement splitText#241
Conversation
ameerf-wix
left a comment
ameerf-wix
left a comment
There was a problem hiding this comment.
Went through everything but the tests. Will update after I go through them too.
| | Option | Type | Default | Description | | ||
| | ------------------ | ---------------------------------------------------------------- | ------------ | ------------------------------------------------------------------------- | | ||
| | `type` | `'chars' \| 'words' \| 'lines' \| 'sentences'` or array of those | — | Split eagerly on call instead of lazily | | ||
| | `wrapperClass` | `string` or `{ chars?, words?, lines?, sentences? }` | — | Extra CSS class(es) on wrapper spans | |
There was a problem hiding this comment.
{ chars?: string, words?: string, lines?: string, sentences?: string }
| if (!this._cache.chars) this._compute('chars'); | ||
| if (this._activeType !== 'chars') this._activate('chars'); | ||
| return this._cache.chars!; | ||
| if (this._domBuilt && this._builtTypes.length >= 2) return this._cache.chars ?? []; |
There was a problem hiding this comment.
this means that it returns empty array when_domBuilt is true and this._builtTypes is ['lines', 'words'] which kind of breaks the promise of the lazy getters.. From what I understand in the documentation, this getter should recompute the split for chars when this getter is used.
There was a problem hiding this comment.
Needs another review, maybe I allowed this as part of the design.
| this._detachObservers(); | ||
| this._resetState(); |
| const merged = this._builtTypes.length | ||
| ? [...new Set([...this._builtTypes, requested])] | ||
| : [requested]; |
There was a problem hiding this comment.
this.builtTypes is always an array - can't this be simply
const merged = [...new Set([...this._builtTypes, requested])] ?
| ); | ||
| } | ||
|
|
||
| function mergeSpans( |
There was a problem hiding this comment.
suggesting this because from the current name I expected to see some clever merging of splitting of different types. The actual code is more like concatenating consecutive split spans
| if (rest.length === 1 && rest[0] === 'chars') { | ||
| mergeSpans(spansByType, nestCharsInsideSpan(wordSpan, options, offsets)); | ||
| } else { | ||
| const inner = buildNestedNodes(innerText, rest, options, offsets); | ||
| wordSpan.textContent = ''; | ||
| wordSpan.append(...inner.nodes); | ||
| mergeSpans(spansByType, inner.spansByType); | ||
| } |
There was a problem hiding this comment.
exactly the same code as in 184-191
There was a problem hiding this comment.
Refactor to remove duplicate code if possible
2 participants
Description