Comments formatting UI #2076
Comments formatting UI #2076
Conversation
# Conflicts: # packages/liveblocks-react-ui/src/components/Composer.tsx # packages/liveblocks-react-ui/src/primitives/Composer/contexts.ts # packages/liveblocks-react-ui/src/primitives/Composer/index.tsx # packages/liveblocks-react/src/_private.ts
SugarDarius
left a comment
SugarDarius
left a comment
There was a problem hiding this comment.
I'm not sure I'm the best person to review but from what I saw it looks dope 🚀
Huge work on this one 👏🏻
| const label = useMemo(() => { | ||
| return $.COMPOSER_TOGGLE_MARK(mark); | ||
| }, [$, mark]); |
There was a problem hiding this comment.
NB: it looks like a bit overkill to get the label with a useMemo here from what I understand. useMemo is expensive to use and like $ is already a memoized value (even if it should be a stable reference instead) I do not think it's really worth to memoize the label here. But I could be wrong ^^
There was a problem hiding this comment.
I do have a tendency to overuse useMemo/useCallback but here the COMPOSER_TOGGLE_MARK override can be set by users so it might be unexpected to run it on every render. I don't think anyone would put super expensive logic in a localization function but still I feel safer controlling its number of invokations
|
Actually, I have thoughts on the styling. The icons are quite faded out by default, and almost look |
|
@CTNicholas We're somewhat limited by our existing tokens: toggles.mp4 |
| export function useOnComposerEditorChangeWithEventSource( | ||
| editorChangeEventSource: EventSource<void>, | ||
| callback: () => void | ||
| ) { |
There was a problem hiding this comment.
Is this hook exported purely because we want to also listen to editor onchange events inside Composer.Form?
There was a problem hiding this comment.
In the Composer.Form parent but also in children components (e.g. ComposerEditorMentionSuggestionsWrapper, ComposerEditorFloatingToolbarWrapper, etc).
| useOnComposerEditorChangeWithEventSource(editorChangeEventSource, () => { | ||
| setMarks(getMarks(editor)); | ||
| }); |
There was a problem hiding this comment.
Related to this comment, if we do not want to export a hook just for this use case, we could maybe make this a useEffect and mimic the implementation of useOnComposerEditorChangeWithEventSource directly here.
There was a problem hiding this comment.
The Composer's index.tsx file is already pretty long and complex so I wanted to avoid adding noise just for subscribing, I simplified this by making a single and generic useEventSource hook.
nimeshnayaju
left a comment
nimeshnayaju
left a comment
There was a problem hiding this comment.
Amazing job @marcbouchenoire 🚀 . This was quite a big undertaking and you did a great job - the final product looks beautiful! I've tried to review this based on my limited Slate.js knowledge and I only have some general questions.
| export function useEventSource<T>( | ||
| eventSource: EventSource<T>, | ||
| callback: () => void | ||
| ) { | ||
| const latestCallback = useLatest(callback); | ||
|
|
||
| useEffect(() => { | ||
| const unsubscribe = eventSource.subscribe(() => latestCallback.current()); | ||
|
|
||
| return unsubscribe; | ||
| }, [eventSource, latestCallback]); | ||
| } |
There was a problem hiding this comment.
Nice! I like this generic hook!
One suggestion here is to only have it rely on the Observable API, not the full EventSource API, which should ideally be an implementation detail used by the source of the events — i.e. where those events get emitted. Any party interested in listening to those events, are better off just relying on the Observable API.
| export function useEventSource<T>( | |
| eventSource: EventSource<T>, | |
| callback: () => void | |
| ) { | |
| const latestCallback = useLatest(callback); | |
| useEffect(() => { | |
| const unsubscribe = eventSource.subscribe(() => latestCallback.current()); | |
| return unsubscribe; | |
| }, [eventSource, latestCallback]); | |
| } | |
| export function useObservable<T>( | |
| observable: Observable<T>, | |
| callback: () => void | |
| ) { | |
| const latestCallback = useLatest(callback); | |
| useEffect(() => { | |
| const unsubscribe = observable.subscribe(() => latestCallback.current()); | |
| return unsubscribe; | |
| }, [observable, latestCallback]); | |
| } |
# Conflicts: # CHANGELOG.md # packages/liveblocks-react-ui/src/index.ts
This PR introduces a new floating toolbar in the Comments composer to allow visual formatting. It's available by default in the high-level
Composercomponent and can be opted-in with theComposer.*primitives. The PR also contains formatting-related APIs to allow building formatting toggles and similar controls.composer-floating-island.mp4
In addition to the technical aspect, I really struggled with naming so if you have any opinions on "FloatingToolbar", "marks", etc, please share them! For "FloatingToolbar" I was thinking of "FormattingToolbar" as well, and for "marks", I initially thought it was too Slate/internal-related but my alternative ("text formats") felt too convoluted and I found out that "marks" is also used by ProseMirror/Tiptap and Lexical.
Fixes LB-254
Fixes LB-1323
Follow-up: #2085