-
Notifications
You must be signed in to change notification settings - Fork 113
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
Upg: improve builder UX when using a template for instructions #6722
Conversation
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.
LGTM
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 gif is 🔥, left a global comment as it might create too many re-renderings.
}, | ||
}); | ||
const editorService = useInstructionEditorService(editor); | ||
|
||
const [letterIndex, setLetterIndex] = useState(0); | ||
|
||
useEffect(() => { |
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.
Looking more at this, it's pretty heavy re-rendering. For each letter we will re-render this component. Either you move this outside of this component so the useEffect is not part of a rendered component but only uses the TipTap API to play the animation. This use case is also often achieve using requestanimationframe
. Also I'm wondering if we could not leverage CSS styles here, with something like this:
editor.chain()
.setContent(`<span class="typewriter-effect">${textContent}</span>`)
.focus("end")
.run();
// Set editable to false while typewriter effect is running
editor.setEditable(false);
// Once the animation is complete, set editable to true
const animationDuration = 4000; // Duration should match the CSS animation duration
const timeoutId = setTimeout(() => {
editor.setEditable(true);
setDoTypewriterEffect(false);
}, animationDuration);
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.
Added some comments and we transcript our findings in PR description.
342b083
to
95c168b
Compare
* Upg: improve builder UX when using a template for instructions * fix lint * Add: comment about re-rendering cost
Description
Animate as a typewriter the instructions for an assistant when using a template, then focus on it and put cursor at the end.
In respect to this comment we investigated a bit more and found a couple things:
editor
fromuseEditor
will mutate (eg: as part of hook dependencies) on content change (doc)xxxEditorService
) will probably not do what we expect.We decided to keep the implementation like that for now and revisit later.
Enregistrement.de.l.ecran.2024-08-09.a.09.21.16.mov
Risk
A bug could led the input box to be uneditable.
Deploy Plan
Deploy
front