-
-
Notifications
You must be signed in to change notification settings - Fork 3k
impr: enable dots typed effect for ligature languages (@byseif21) #7458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ca7aca3 to
e051784
Compare
ded97b2 to
fb8df2e
Compare
fb8df2e to
aae08c9
Compare
| "colorfulMode", | ||
| "showAllLines", | ||
| "fontSize", | ||
| "fontFamily", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now run updateWordWrapperClasses unnecessarily when a user changes fontFamily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this?
if (key !== "fontFamily"){
updateWordWrapperClasses();
}
Ligatures.update(key, wordsEl);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if ok? or heavy and should we do batching instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do this:
words.forEach((word) => {
if (shouldReset) reset(word);
applyIfNeeded(word);
})
(not tested)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i meant performance wise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about batching then? too much or worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider replacing with this:
return wordEl.getParent().hasClass("withLigatures");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? closest faster and better, getParent() creates a new ElementWithUtils object wrapper every single time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do the spin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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