Add ability to combine attachments and message text in a single activity #5084

ms-jb · 2024-03-22T19:11:15Z

Fixes #5083

Changelog Entry

Resolves #5083. Added sendAttachmentOn style option to send attachments and text in a single activity, by @ms-jb

Description

Added a new styleOption that enables attachments and message text to be sent in a single activity. If enabled, uploaded files are retained until the Send button is pressed to allow for message text to be optionally included. Standalone text works as before.

Design

Sample implementation:

<script>
  window.WebChat.renderWebChat(
    {
      styleOptions: {
        sendAttachmentOn: 'send' // Default
      }
    },
    document.getElementById('webchat')
  );
</script>

Specific Changes

Added combineAttachmentsAndText style option to send attachments and text in a single activity

I have added tests and executed them locally
I have updated CHANGELOG.md
I have updated documentation

Review Checklist

This section is for contributors to review your work.

Accessibility reviewed (tab order, content readability, alt text, color contrast)
Browser and platform compatibilities reviewed
CSS styles reviewed (minimal rules, no z-index)
Documents reviewed (docs, samples, live demo)
Internationalization reviewed (strings, unit formatting)
package.json and package-lock.json reviewed
Security reviewed (no data URIs, check for nonce leak)
Tests reviewed (coverage, legitimacy)

…hment-text

compulim · 2024-03-25T10:49:12Z

Please get sign off from designer.

…hment-text

ms-jb · 2024-03-29T22:53:57Z

Will update the icons to match the new design:

ms-jb · 2024-04-01T15:49:57Z

@compulim I've updated this PR to match the new design here: https://www.figma.com/file/ccXBJpMPQZCfE9nV8RjIfO/Test-Chat-for-Build?type=design&node-id=4269-20068&mode=design&t=K3emilPqELja2jEQ-0

compulim · 2024-04-03T09:25:53Z

packages/component/src/SendBox/UploadButton.tsx

+      setFiles(files);
+
+      if (combineAttachmentsAndText) {
+        current.blur();


We can't just blur the focus. It will impact accessibility.

compulim · 2024-04-03T09:26:29Z

packages/component/src/providers/internal/SendBox/SendBoxComposer.tsx

  const focus = useFocus();
  const localize = useLocalizer();
  const scrollToEnd = useScrollToEnd();
  const styleToEmotionObject = useStyleToEmotionObject();
  const submitErrorMessageId = useUniqueId('webchat__send-box__error-message-id');
  const timeoutRef = useRef<readonly [Timeout, Timeout] | undefined>(undefined);
+  const [{ combineAttachmentsAndText }] = useStyleOptions();


compulim · 2024-04-03T09:26:48Z

packages/component/src/providers/internal/SendBox/SendBoxComposer.tsx

+          sendFiles(files, sendBoxValue);
+          setFiles([]);
+          setSendBoxValue('');
+          if (uploadButtonRef?.current) {


We can't expose DOM element using hooks.

compulim · 2024-04-03T09:27:07Z

packages/component/src/providers/internal/SendBox/SendBoxComposer.tsx

+          setFiles([]);
+          setSendBoxValue('');
+          if (uploadButtonRef?.current) {
+            uploadButtonRef.current.value = null;


Why set the upload file value to null when setFiles([]) remove all the files?

compulim · 2024-04-03T09:28:40Z

packages/component/src/providers/internal/SendBox/SendBoxComposer.tsx

-        : undefined
+      : // If combineAttachments is enabled, allow sending if either there is a message or files
+        // Otherwise, require message text only
+        (combineAttachmentsAndText && (sendBoxValue || files.length)) || (!combineAttachmentsAndText && sendBoxValue)


This logic seems too complicated.

compulim · 2024-04-03T09:29:07Z

packages/core/src/actions/sendFiles.js

@@ -1,9 +1,9 @@
 const SEND_FILES = 'WEB_CHAT/SEND_FILES';

-export default function sendFiles(files) {
+export default function sendFiles(files, text) {


Please convert this file to TypeScript.

packages/core/src/sagas/sendFilesToPostActivitySaga.js

compulim · 2024-04-03T09:34:26Z

packages/component/src/SendBox/UploadButton.tsx

        role="button"
        tabIndex={-1}
        type="file"
      />
      <IconButton alt={uploadFileString} aria-label={uploadFileString} disabled={disabled} onClick={handleClick}>
        <AttachmentIcon />
+        {/* When a file is attached, overlay the check icon */}
+        {uploadButtonRef.current?.value && <CheckIcon />}


Build an icon, don't overlay.

compulim · 2024-04-03T09:34:47Z

packages/component/src/providers/internal/SendBox/SendBoxComposer.tsx

  const apiSubmitSendBox = useSubmitSendBox();
+  const [{ files, setFiles }] = useFiles();


compulim · 2024-04-03T09:35:31Z

packages/component/src/providers/internal/SendBox/SendBoxComposer.tsx

+      apiSubmitSendBox,
+      clearTimeout,
+      combineAttachmentsAndText,
+      files,


Any changes to files should not recreate the callback.

Use ref.

compulim · 2024-04-03T10:34:13Z

packages/api/src/StyleOptions.ts

@@ -270,6 +270,10 @@ type StyleOptions = {
   * @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#multiple
   */
  uploadMultiple?: boolean;
+  /**
+   * Send the attachments and message text together as a single activity


Suggested change

* Send the attachments and message text together as a single activity

* Send the attachments and message text together as a single activity.

compulim · 2024-04-03T10:53:21Z

packages/component/src/SendBox/UploadButton.tsx

        role="button"
        tabIndex={-1}
        type="file"
      />
      <IconButton alt={uploadFileString} aria-label={uploadFileString} disabled={disabled} onClick={handleClick}>
        <AttachmentIcon />
+        {/* When a file is attached, overlay the check icon */}
+        {uploadButtonRef.current?.value && <CheckIcon />}


Using uploadButtonRef.current?.value to check if a file is pending upload, is probably the reason why you need to set uploadButtonRef.current.value to null after send.

It seems useFiles hook was added much later and not really used.

compulim · 2024-04-03T11:26:59Z

packages/api/src/hooks/useFiles.ts

+/**
+ * This is a hook that returns the files and setFiles from the context.
+ */
+export default function useFiles(): [{ files: File[]; setFiles: (files: File[]) => void }] {


useFiles should follow the pattern of other state hooks:

useFiles(): [readonly File[], (readonly File[]) => void]

Renamed to useSendBoxAttachments.

compulim · 2024-04-03T11:27:27Z

packages/api/src/hooks/useFiles.ts

+import useWebChatAPIContext from './internal/useWebChatAPIContext';
+
+/**
+ * This is a hook that returns the files and setFiles from the context.


Add more context.

compulim · 2024-04-03T11:28:16Z

packages/api/src/hooks/useFiles.ts

+/**
+ * This is a hook that returns the files and setFiles from the context.
+ */
+export default function useFiles(): [{ files: File[]; setFiles: (files: File[]) => void }] {


The name "file" is very vague. It could be files from an incoming activity. Or files to send out.

compulim · 2024-04-03T11:32:58Z

packages/component/src/SendBox/UploadButton.tsx

+        focus('sendBox');
+        return;
+      }
+
      sendFiles(files);


Should use the SendBoxComposer.useSubmit() hook to send files. This will simplify lots of code.

compulim · 2024-04-03T11:34:45Z

packages/component/src/providers/internal/SendBox/SendBoxComposer.tsx

@@ -116,10 +132,33 @@ const SendBoxComposer = ({ children }: PropsWithChildren<{}>) => {
        ]) as readonly [Timeout, Timeout];
      } else {
        scrollToEndRef.current?.();
-        apiSubmitSendBox();
+        if (combineAttachmentsAndText && files.length) {


The old logic use Redux to send, while the new logic use the hook directly. This is inconsistent.

compulim · 2024-04-03T11:35:56Z

packages/component/src/providers/internal/SendBox/SendBoxComposer.tsx

@@ -116,10 +132,33 @@ const SendBoxComposer = ({ children }: PropsWithChildren<{}>) => {
        ]) as readonly [Timeout, Timeout];
      } else {
        scrollToEndRef.current?.();
-        apiSubmitSendBox();
+        if (combineAttachmentsAndText && files.length) {


Few lines above, it already checked if there are any errors. One type of error is "no files attached".

The check here (files.length) is meaningless.

compulim · 2024-04-03T11:39:33Z

packages/api/src/hooks/Composer.tsx

@@ -540,6 +540,12 @@ const ComposerCore = ({
    [scrollToEndButtonMiddleware]
  );

+  // For useFiles hook
+  const [files, setFiles] = useState<File[]>([]);


Using useState here, instead of Redux, will complicate the code later.

In your latter code, sometimes, it call useSendFiles, and in other times, it call, useAPISubmitSendBox. This complicate testing.

If the attachment list is maintained inside Redux, the latter code will not need to call useSendFiles.

We could move away from Redux too. But we should select one path. Doing 1 thing with 2 ways will slowly introduce branching/versioning issue.

compulim · 2024-04-03T13:02:46Z

packages/api/src/hooks/useSendFiles.ts

@@ -12,14 +12,14 @@ type PostActivityFile = {
  url: string;
 };

-export default function useSendFiles(): (files: PostActivityFile[]) => void {
+export default function useSendFiles(): (files: PostActivityFile[], text?: string) => void {


When text argument is introduced to useSendFiles, it almost act the same as useSendMessage.

Instead of adding text argument to useSendFiles, should add attachments to useSendMessage instead. Then, useSendFiles can be deprecated.

Added deprecation notes. Still need a warn once.

Incorporate PR changes

compulim · 2024-04-08T17:16:37Z

Obsoleted by PR #5123.

ms-jb added 22 commits March 7, 2024 16:51

Add style option to allow for file type narrowing

88c983a

Rename and add multi upload prop

69b9cbe

Fix launch config type

2101847

Add e2e test to validate <input>

6305629

Resolve comments

5ed4b0d

Revert to true

387f006

Resolve e2e test

e57ca0b

Added changelog entry

c85ffac

Add style option

cda5283

Add hook

c1c830e

Pass text through to activity

cade807

Add file state to context

1f625d2

Pass text to sendFiles

4ed2f25

Finalize upload and sendbox action

bf0283d

Merge remote-tracking branch 'origin/main' into feature/combine-attac…

45e1126

…hment-text

Fix bad merge

9d9d4f5

Only send text if defined

c5ff229

Add changelog entry

f255971

Add e2e test

ac834e7

Bump version

67938fa

Switch to check icon

56b7ad9

Initialize as undefined

c88627d

ms-jb requested review from a-b-r-o-w-n, compulim, cwhitten, srinaath, tdurnford and beyackle2 as code owners March 22, 2024 19:11

Store the upload button ref in context and clear after sending

2c72d8a

ms-jb added 4 commits March 27, 2024 09:57

Fix types and pass ref directly to send box

1b326ab

Update icon and sizing

230f2e3

Smaller icon

0ba536e

Merge remote-tracking branch 'origin/main' into feature/combine-attac…

09f5a3d

…hment-text

Updated send box icons per design

3909c6f

compulim reviewed Apr 3, 2024

View reviewed changes

compulim and others added 4 commits April 3, 2024 13:55

Use Redux and sendMessage, instead of useState and sendFile

218033f

Reformat

3030d4f

Update DOCTYPE

decd63b

Merge pull request #1 from compulim/feat-pending-upload

c91cfbb

Incorporate PR changes

compulim closed this Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to combine attachments and message text in a single activity #5084

Add ability to combine attachments and message text in a single activity #5084

ms-jb commented Mar 22, 2024 •

edited

Loading

compulim commented Mar 25, 2024

ms-jb commented Mar 29, 2024

ms-jb commented Apr 1, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024 •

edited

Loading

compulim Apr 3, 2024

compulim Apr 3, 2024

compulim Apr 3, 2024 •

edited

Loading

compulim Apr 3, 2024 •

edited

Loading

compulim Apr 3, 2024

compulim Apr 3, 2024 •

edited

Loading

compulim commented Apr 8, 2024

		const apiSubmitSendBox = useSubmitSendBox();
		const [{ files, setFiles }] = useFiles();

	* Send the attachments and message text together as a single activity
	* Send the attachments and message text together as a single activity.

Add ability to combine attachments and message text in a single activity #5084

Add ability to combine attachments and message text in a single activity #5084

Conversation

ms-jb commented Mar 22, 2024 • edited Loading

Changelog Entry

Description

Design

Specific Changes

Review Checklist

compulim commented Mar 25, 2024

ms-jb commented Mar 29, 2024

ms-jb commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compulim Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compulim Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

compulim Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compulim Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

compulim commented Apr 8, 2024

ms-jb commented Mar 22, 2024 •

edited

Loading

compulim Apr 3, 2024 •

edited

Loading

compulim Apr 3, 2024 •

edited

Loading

compulim Apr 3, 2024 •

edited

Loading

compulim Apr 3, 2024 •

edited

Loading