impr: enable dots typed effect for ligature languages (@byseif21)#7458
impr: enable dots typed effect for ligature languages (@byseif21)#7458byseif21 wants to merge 14 commits intomonkeytypegame:masterfrom
Conversation
ca7aca3 to
e051784
Compare
ded97b2 to
fb8df2e
Compare
fb8df2e to
aae08c9
Compare
| "colorfulMode", | ||
| "showAllLines", | ||
| "fontSize", | ||
| "fontFamily", |
There was a problem hiding this comment.
We now run updateWordWrapperClasses unnecessarily when a user changes fontFamily.
There was a problem hiding this comment.
the thing is I think I still need some of the layout updates it triggers when fontFamily changes. I’ll give it some manual testing to see
There was a problem hiding this comment.
Can we do this?
if (key !== "fontFamily"){
updateWordWrapperClasses();
}
Ligatures.update(key, wordsEl);
There was a problem hiding this comment.
kinda feel like fontFamily should also trigger updateWordWrapperClasses but can’t really prove it visually enough till now, so yeah ok I’ll leave it limited for now
| if (shouldReset) { | ||
| words.forEach(reset); | ||
| } | ||
| words.forEach(applyIfNeeded); |
There was a problem hiding this comment.
check if ok? or heavy and should we do batching instead?
There was a problem hiding this comment.
I'd do this:
words.forEach((word) => {
if (shouldReset) reset(word);
applyIfNeeded(word);
})
(not tested)
There was a problem hiding this comment.
i meant performance wise
There was a problem hiding this comment.
Performance wise we're running on the words twice when shouldReset is true, while we can do it one pass. I don't see why go with this approach rather than doing it in one loop.
There was a problem hiding this comment.
what do you think about batching then? too much or worth it?
There was a problem hiding this comment.
I'd say it's worth it.
| if (Config.typedEffect !== "dots") return false; | ||
| if (wordEl.hasClass("broken-ligatures")) return false; | ||
|
|
||
| return !!wordEl.native.closest(".withLigatures"); |
There was a problem hiding this comment.
Consider replacing with this:
return wordEl.getParent().hasClass("withLigatures");
There was a problem hiding this comment.
why? closest faster and better, getParent() creates a new ElementWithUtils object wrapper every single time
There was a problem hiding this comment.
closest seems less readable, as it doesn't make it clear what element might have the withLigatures class. Also (it might just be me), but I really hate the !! syntax.
There was a problem hiding this comment.
I lost, so keep current approach.
fix the limitation of dots effect wouldn't work correctly for ligature-based languages cuz connected letters cannot be rendered as individual dots.
before it was not worth the fix for the theme only now after the new typed effect feat #7360 i think this needed.
introduces a small helper that breaks ligatures only after a word is finished, allowing the dots effect to render correctly while keeping ligatures intact during typing.
runs only when needed (on word completion or when the typed effect is switched to dots) and avoids any continuous checks or performance overhead.
related #6472