fix: chat composer examples for handle change events #3426
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
🦋 Changeset detectedLatest commit: b7ae455 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
Status: Do Not Merge
github-actions
bot
added
Type: Documentation
SiTaggart
commented
Aug 17, 2023
| import merge from 'deepmerge'; | ||
|
|
||
| import {chatComposerLexicalStyles} from './styles'; | ||
| import {AutoLinkPlugin} from './AutoLinkPlugin'; | ||
| import {PlaceholderWrapper} from './PlaceholderWrapper'; | ||
| import {baseConfig, renderInitialText} from './helpers'; | ||
|
|
||
| export interface ChatComposerProps extends ContentEditableProps { | ||
| export interface ChatComposerProps extends Omit<ContentEditableProps, 'style' | 'className' | 'onChange'> { |
There was a problem hiding this comment.
Rather than set them to never, just don't allow them. And new onChange types from ContentEditable clash so remove that too
✅ Deploy Preview for paste-theme-designer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
SiTaggart
commented
Aug 17, 2023
| children?: LexicalComposerProps['children']; | ||
| config: LexicalComposerProps['initialConfig']; | ||
| element?: BoxProps['element']; | ||
| placeholder?: string | JSX.Element; |
There was a problem hiding this comment.
unsure on this one, it clashes now and I don't really know why we would change it from what is supplied in the ContentEditable types
✅ Deploy Preview for paste-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
SiTaggart
commented
Aug 17, 2023
| maxHeight?: BoxStyleProps['maxHeight']; | ||
| initialValue?: string; | ||
| disabled?: boolean; | ||
|
|
||
| /** Mapped to ariaOwneeID on the Lexical ContentEditable component. */ | ||
| ariaOwns?: ContentEditableProps['ariaOwneeID']; |
There was a problem hiding this comment.
They've fixed all these issues now, so no need to rewire them. And we handle the banned props
SiTaggart
commented
Aug 17, 2023
| @@ -6,7 +6,7 @@ import { | |||
| MinimizableDialogHeader, | |||
| MinimizableDialogContent, | |||
| } from '@twilio-paste/minimizable-dialog'; | |||
| import {AutoScrollPlugin, $getRoot, ClearEditorPlugin} from '@twilio-paste/lexical-library'; | |||
| import {$getRoot, ClearEditorPlugin} from '@twilio-paste/lexical-library'; | |||
There was a problem hiding this comment.
AutoScrollPlugin is removed from lexical
☁️ Nx Cloud ReportCI is running/has finished running commands for commit b7ae455. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
SiTaggart
commented
Aug 17, 2023
| @@ -121,6 +120,7 @@ export const ConversationsUIKitExample: StoryFn = () => { | |||
| }; | |||
|
|
|||
| const submitMessage = (): void => { | |||
| if (message === '') return; | |||
There was a problem hiding this comment.
prevent empty bubbles
SiTaggart
commented
Aug 17, 2023
| @@ -132,11 +132,10 @@ export const ConversationsUIKitExample: StoryFn = () => { | |||
| <MinimizableDialog aria-label="Live chat"> | |||
| <MinimizableDialogHeader>Live chat</MinimizableDialogHeader> | |||
| <MinimizableDialogContent> | |||
| <Box overflowY="scroll" maxHeight="size50" tabIndex={0}> | |||
| <Box overflow="hidden" overflowY="scroll" maxHeight="size50" tabIndex={0}> | |||
There was a problem hiding this comment.
Stops the weird scroll behaviour as you animate in new bubbles
There was a problem hiding this comment.
But does it scroll? [edit] Seems like it does.
SiTaggart
commented
Aug 17, 2023
| @@ -41,6 +41,7 @@ export const ChatBubble = React.forwardRef<HTMLDivElement, ChatBubbleProps>( | |||
| element={element} | |||
| ref={ref} | |||
| variant={variant} | |||
| whiteSpace="pre-wrap" | |||
There was a problem hiding this comment.
Solves the line breaks in user generated strings
SiTaggart
commented
Aug 17, 2023
Passing run #6616 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|||||||||||||||
|
Size Change: +10.7 kB (+1%) Total Size: 1.01 MB
ℹ️ View Unchanged
|
✅ Deploy Preview for paste-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b7ae455:
|
TheSisb
previously approved these changes
Aug 18, 2023
| "lexical": "0.6.0" | ||
| "@lexical/link": "0.12.0", | ||
| "@lexical/react": "0.12.0", | ||
| "lexical": "0.12.0" |
| @@ -41,6 +41,7 @@ export const ChatBubble = React.forwardRef<HTMLDivElement, ChatBubbleProps>( | |||
| element={element} | |||
| ref={ref} | |||
| variant={variant} | |||
| whiteSpace="pre-wrap" | |||
| @@ -132,11 +132,10 @@ export const ConversationsUIKitExample: StoryFn = () => { | |||
| <MinimizableDialog aria-label="Live chat"> | |||
| <MinimizableDialogHeader>Live chat</MinimizableDialogHeader> | |||
| <MinimizableDialogContent> | |||
| <Box overflowY="scroll" maxHeight="size50" tabIndex={0}> | |||
| <Box overflow="hidden" overflowY="scroll" maxHeight="size50" tabIndex={0}> | |||
There was a problem hiding this comment.
But does it scroll? [edit] Seems like it does.
serifluous
reviewed
Aug 18, 2023
serifluous
reviewed
Aug 18, 2023
packages/paste-website/src/pages/components/chat-composer/index.mdx
Outdated
Show resolved
Hide resolved
serifluous
reviewed
Aug 18, 2023
packages/paste-website/src/pages/components/chat-composer/index.mdx
Outdated
Show resolved
Hide resolved
serifluous
reviewed
Aug 18, 2023
packages/paste-website/src/pages/components/chat-composer/index.mdx
Outdated
Show resolved
Hide resolved
serifluous
reviewed
Aug 18, 2023
packages/paste-website/src/pages/components/chat-composer/index.mdx
Outdated
Show resolved
Hide resolved
serifluous
reviewed
Aug 18, 2023
SiTaggart
commented
Aug 22, 2023
| expect(screen.getByRole('textbox')).toBeDefined(); | ||
| expect(screen.getByText('Type here..')).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should pass props to the content editable', async () => { | ||
| render( | ||
| <ChatComposer | ||
| testid="my-composer" | ||
| data-testid="my-composer" |
There was a problem hiding this comment.
I have no idea how these tests were actually working. testid isn't a valid html prop, it should never have worked.
Co-authored-by: Sarah <sali@twilio.com>
SiTaggart
force-pushed
the
chore/chat-log-exmaples
branch
from
58cd0c5
to
7e5e16c
Compare
serifluous
approved these changes
Aug 23, 2023
SiTaggart
added
🚀 merge it!
🕵🏻♀️ Run website visual regression
TheSisb
approved these changes
Aug 23, 2023
| expect(screen.getByRole('textbox')).toBeDefined(); | ||
| expect(screen.getByText('Type here..')).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should pass props to the content editable', async () => { | ||
| render( | ||
| <ChatComposer | ||
| testid="my-composer" | ||
| data-testid="my-composer" |
| export const createNewMessage = (message: string): Omit<Chat, 'id'> => { | ||
| const time = new Date().toLocaleString('en-US', { | ||
| hour: 'numeric', | ||
| minute: 'numeric', | ||
| hour12: true, | ||
| }); | ||
|
|
||
| const messageDirection = getRandomInt(2) === 1 ? 'inbound' : 'outbound'; |
There was a problem hiding this comment.
not super important but I woulda done Math.random() > 0.5 ?
| }, []); | ||
|
|
||
| React.useEffect(() => { | ||
| if (!mounted || !loggerRef.current) return; |
There was a problem hiding this comment.
IIRC anything in a useEffect will only run after mount. Do we need the mounted variable at all? I would figure we can remove it. If you had an unmount operation in the previous useEffect like return () => setMounted(false) then I may not have noticed/commented.
[edit] Oh unless this is only supposed to run on updates and never on mounts. That makes sense now.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description and useful links to discussions / issues / tickets
Changes in this PR:
Update lexical
Its old and some stuff has been deprecated
Update Chat Composer
Accomodate the new version of Lexical, remove the deprecated AutoScrollPlugin and tidy types
Update Chat Log
Better handling of line breaks in the Chat bubble. Improvements to the ChatLogger to keep new messages in view as they are added to the log, to make better website examples.
Checklist
requiredchecks have passed🕵🏻♀️ Run website visual regressionlabel