Possible modification for better code #97
Replies: 1 comment
-
Agreed. We should short-circuit the lookup and can have this around conditional to avoid unnecessarily DOM access. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
https://github.com/Progyan1997/Operational-Transformation/blob/c743cc51648d544c1a783b66e0c81dc208e0e593/packages/ace/src/tooltip-marker.ts#L52
Hey @Progyan1997 , I thought about this code here and I feel that this piece of code works for now but depends on
querySelector
to retrieve theace_content
div. Instead of that I think we can shift this code inside theupdate
method and useclosest
to append the tooltip marker like so :-where
_aceContent
is a new property onTooltipMarker
class. We can also not introduce this new property and instead run theclosest
andappend
operation every time which will result in same behavior.What do you think ? If you would wanna go with this, would you choose introducing
_aceContent
or performingclosest
andappend
every time ?My intuition for this change - The reason is possible collision of same class names can break this with top to bottom existing approach. In bottom to top with
closest
the probability is very less unless user manipulates the editor DOM nodes itself.Beta Was this translation helpful? Give feedback.
All reactions