feat: add new file-input component #6355

robinzigmond · 2023-10-09T14:06:39Z

Only supports input of a single file at present - multiple mode will follow in the future.

Proposed behaviour

A FileInput component should exist in the Carbon library, matching the Design System designs.

Current behaviour

There is no FileInput component

Checklist

QA

Tested in CodeSandbox/storybook
Add new Playwright test coverage if required
Carbon implementation matches Design System/designs
UI Tests GitHub check reviewed if required

Additional context

Testing instructions

The following CodeSandbox is an example of the broken behaviour.
You can see the new behaviour by looking at the version in the comment by codesandbox[bot].

codesandbox-ci · 2023-10-09T14:09:13Z

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 ecbc2fe:

Sandbox	Source
carbon-quickstart	Configuration
carbon-quickstart-typescript	Configuration
carbon-quickstart	PR

edleeks87

Great work @robinzigmond 👍. I've noticed some potentially minor styling that might need updating but as you have a UX review/ chat with Ds planned I'll leave that stuff up to them as I've confirmed it all functions as I'd expect etc

src/components/file-input/file-input.stories.mdx

src/components/file-input/file-input.component.tsx

src/__internal__/utils/helpers/tags/tags-specs/tags-specs.ts

src/components/file-input/file-input.stories.tsx

src/components/file-input/file-input.style.tsx

nineteen88

Praise: Looks really good Robin. Top work!

Just a few nitpicky comments more than anything

nineteen88 · 2023-10-12T14:55:34Z

src/components/file-input/file-input.stories.tsx

+  return <FileInput label="File input" />;
+};
+
+export const Margins: ComponentStory<typeof FileInput> = () => {


suggestion(non-blocking): I'm not sure on the value of this particular story

The other stories you have added make sense in terms of a consumer might be wondering how to do this different approaches with the File Input component, but I think Margins are pretty self explanatory and you can see Margins are supported in the prop table. There are lot of stories to contend with so I think removing bloat is good. Up to you though.

Fair comment. I added it because basically the first several stories I added were "copies" of those in Textbox that still seemed relevant for FileInput. I wasn't even sure whether to have the Margin Props, or whether I should add Padding Props too - I expected quite a discussion on that tbh 😁 I'll hold my hands up and say I just went for consistency with Textbox (which also has this story), without thinking about it too much - that could be the wrong approach as they're obviously pretty different as components!

Having made my excuses, I agree this story isn't really adding much for consumers so happy to remove it 👍

nineteen88 · 2023-10-12T16:22:07Z

src/components/file-input/file-input.stories.mdx

+on the client side, resulting in storing the file locally as a `data:` URL. This is a way to show users a preview of their file if you do not intend
+to track upload progress on the server, or do not actually send the files to a server until form submission.
+
+Note the use of a `ref` to store the `FileReader` object - this is not essential but avoids having to create a new such object more than once in the


nitpick (non-blocking): Remove the word such

Suggested change

Note the use of a `ref` to store the `FileReader` object - this is not essential but avoids having to create a new such object more than once in the

Note the use of a `ref` to store the `FileReader` object - this is not essential but avoids having to create a new object more than once in the

Yeah, I agree it reads awkwardly with that word in 👍 Not sure if I like just removing it though, as "a new object" isn't really being specific enough imo - the idea was to basically say "a new FileReader object" without repeating myself. I'll have a bit of a play and hopefully come up with a better way of phrasing it.

nineteen88 · 2023-10-12T16:24:09Z

src/components/file-input/file-input.stories.mdx

+  <Story name="tracking upload status on client side" story={stories.UploadStatusClient} />
+</Canvas>
+
+This example mocks an alternative approach where files added to the file input are immediately sent to the server, and the UI tracks the progress of this.


nitpick (non-blocking): The double this reads a little weird

Suggested change

This example mocks an alternative approach where files added to the file input are immediately sent to the server, and the UI tracks the progress of this.

This example mocks an alternative approach where files added to the file input are immediately sent to the server, and the UI tracks the progress.

nineteen88 · 2023-10-12T16:25:02Z

src/components/file-input/file-input.stories.mdx

+
+This example mocks an alternative approach where files added to the file input are immediately sent to the server, and the UI tracks the progress of this.
+This simple example mocks the progress (and the possibility of errors, such as network errors) with random numbers - in a real scenario this would poll an
+alternative endpoint for the upload progress, or use WebSocket messages sent from the serber to update progress.


nitpick (non-blocking): Incorrect spelling

Suggested change

alternative endpoint for the upload progress, or use WebSocket messages sent from the serber to update progress.

alternative endpoint for the upload progress, or use WebSocket messages sent from the server to update progress.

nineteen88 · 2023-10-12T16:34:46Z

src/components/file-input/__internal__/file-upload-status/file-upload-status.component.tsx

+  onAction,
+  ...statusProps
+}: FileUploadStatusProps) => {
+  const l = useLocale();


suggestion(non-blocking): Use semantic variable names

I think we should avoid using non-descriptive variables names as it doesn't mean anything to someone scanning the code. It would typically be flagged as a code smell but I guess we don't have any static code analysis in that form.

While I completely agree in general, l is what we universally use for this locale object in other components so I was just copying that. I'd be more than happy to change it but I think this needs a wider discussion - not just on what to call it but as to whether I use a different name here or we stick with consistency and perhaps update them all at once in a later piece of work.

Agree with both of you if that's possible, we should be as descriptive as possible but Robin has followed how we have implemented this elsewhere and how we've documented in our own docs so we should look to update them in separate ticket

nineteen88 · 2023-10-12T16:49:09Z

src/components/file-input/file-input.component.tsx

+    }: FileInputProps,
+    ref: React.ForwardedRef<HTMLInputElement>
+  ) => {
+    const l = useLocale();


suggestion(non-blocking): non-descriptive variable name

As mentioned before

same response as above

nineteen88 · 2023-10-12T16:58:45Z

src/components/file-input/file-input.pw.tsx

+
+  const dataTransfer = await page.evaluateHandle(
+    async ({ bufferData, localFileName, localFileType }) => {
+      const dt = new DataTransfer();


suggestion(non-blocking): non-descriptive variable name

As mentioned previously

I'll admit this was basically a copy-paste from a solution I found in someone's comment on a Github issue. I'm happy to change the variable name (to something like dataTransferObject I guess?)

nineteen88 · 2023-10-12T17:01:31Z

src/components/file-input/file-input.spec.tsx

+
+import FileInput, { FileUploadStatusProps } from ".";
+
+describe("FileInput component", () => {


suggestion(non-blocking): superfluous describe block

As mentioned before

nineteen88 · 2023-10-12T17:07:14Z

src/components/file-input/file-input.style.tsx

+
+export const ErrorBorder = styled.span`
+  position: absolute;
+  z-index: 6;


question: what is this z-index used for?

It stood out because 6 seems to be a bit random to me, but I've tried removing it on storybook and it doesn't make a difference.

Excellent question that I can't answer 😅 I'll hold my hand up and admit this was copy-pasted from an equivalent styled component elsewhere (almost certainly Textbox but I don't specifically remember). Similar to what @edleeks87 commented on the hint text, that could arguably be moved to a central location (but as I argued there I'd rather not do it in this PR). Most likely the z-index can be removed from the other ones too, or maybe it's specific to that component that I copied it from and just not needed here?

For now I think I'll keep things separate and just remove the z-index from this component after confirming it's not needed. Thanks for spotting it.

nineteen88 · 2023-10-12T17:12:16Z

src/components/file-input/file-input.style.tsx

+        align-items: center;
+        text-align: center;
+        gap: 12px;
+        border-radius: 4px;


question: could this be a design token?

I'm just wondering if it's possible that there's consistency here that DS want standard rounded corners across components. It's more in case they want to square off corners again it would be easier with a design token... But maybe I'm just worrying about something that won't happen

Good question. I grabbed the ones I could from Figma but mostly it's only colours that that seems to show tokens for. I've now had a catch up with Laia from the DS team and she's pointed out a few tokens I missed (in terms of the colour etc actually being wrong, I mean) and has sent me a full list of them all - so absolutely any styles that come from tokens will be done properly.

in fact, similar to Ed's suggestion above with the StyledHintText, I'm just going to remove this entirely and inport this styled component from textbox.style, as several components already do (and pending a later tidy-up that moves these to a common file).

src/components/file-input/__internal__/file-upload-status/file-upload-status.style.tsx

harpalsingh · 2023-11-10T13:37:29Z

Had an additional UXQA on this after updates and happy with the work again.

carbonci · 2023-12-13T14:49:24Z

🎉 This PR is included in version 124.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

robinzigmond added Pending Review Pending QA Pending UX QA Squad Front End labels Oct 9, 2023

robinzigmond self-assigned this Oct 9, 2023

robinzigmond requested review from a team as code owners October 9, 2023 14:06

robinzigmond marked this pull request as draft October 9, 2023 14:16

edleeks87 self-requested a review October 10, 2023 14:33

nineteen88 self-requested a review October 12, 2023 08:56

edleeks87 reviewed Oct 12, 2023

View reviewed changes

nineteen88 reviewed Oct 12, 2023

View reviewed changes

nineteen88 previously approved these changes Oct 17, 2023

View reviewed changes

edleeks87 reviewed Oct 18, 2023

View reviewed changes

src/components/file-input/__internal__/file-upload-status/file-upload-status.style.tsx Show resolved Hide resolved

robinzigmond force-pushed the FE-6216-file-input branch from 6552c29 to b34e5b0 Compare October 19, 2023 09:43

robinzigmond dismissed nineteen88’s stale review via 42ba78a October 19, 2023 09:44

robinzigmond force-pushed the FE-6216-file-input branch from b34e5b0 to 42ba78a Compare October 19, 2023 09:44

edleeks87 previously approved these changes Oct 19, 2023

View reviewed changes

nineteen88 previously approved these changes Oct 19, 2023

View reviewed changes

nineteen88 added Passed Review Passed QA and removed Pending Review Passed QA labels Oct 19, 2023

robinzigmond marked this pull request as ready for review October 19, 2023 10:48

robinzigmond dismissed stale reviews from nineteen88 and edleeks87 via d838b94 October 23, 2023 10:04

edleeks87 previously approved these changes Oct 23, 2023

View reviewed changes

nineteen88 previously approved these changes Oct 23, 2023

View reviewed changes

edleeks87 previously approved these changes Oct 31, 2023

View reviewed changes

nineteen88 previously approved these changes Oct 31, 2023

View reviewed changes

harpalsingh added Passed UX QA and removed Pending UX QA labels Nov 3, 2023

fix: css syntax for text opacity

6339e5e

robinzigmond dismissed stale reviews from nineteen88 and edleeks87 via 6339e5e November 10, 2023 09:52

fix: correct default min-width and fix docs

f5172ca

robinzigmond added 3 commits November 16, 2023 09:57

feat: ensure text of long file name does not overflow container

8cdabc9

docs: add controls for test stories

d0d5131

docs: add separate subheading for each story

3fdbcb6

nineteen88 previously approved these changes Nov 20, 2023

View reviewed changes

fix: link focus styling

5a75d03

robinzigmond dismissed nineteen88’s stale review via 5a75d03 November 21, 2023 15:07

edleeks87 previously approved these changes Nov 27, 2023

View reviewed changes

nineteen88 previously approved these changes Nov 28, 2023

View reviewed changes

docs: restrict file size in client-side upload example

9388b8c

robinzigmond dismissed stale reviews from nineteen88 and edleeks87 via 9388b8c December 6, 2023 11:35

edleeks87 approved these changes Dec 6, 2023

View reviewed changes

nineteen88 approved these changes Dec 8, 2023

View reviewed changes

divyajindel approved these changes Dec 13, 2023

View reviewed changes

divyajindel added Passed QA and removed Pending QA labels Dec 13, 2023

Merge branch 'master' into FE-6216-file-input

ecbc2fe

robinzigmond merged commit 8e99496 into master Dec 13, 2023
23 checks passed

robinzigmond deleted the FE-6216-file-input branch December 13, 2023 14:44

carbonci added the released label Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new file-input component #6355

feat: add new file-input component #6355

robinzigmond commented Oct 9, 2023 •

edited by divyajindel

Loading

codesandbox-ci bot commented Oct 9, 2023 •

edited

Loading

edleeks87 left a comment

nineteen88 left a comment

nineteen88 Oct 12, 2023

robinzigmond Oct 13, 2023

nineteen88 Oct 12, 2023

robinzigmond Oct 13, 2023

nineteen88 Oct 12, 2023

nineteen88 Oct 12, 2023

nineteen88 Oct 12, 2023

robinzigmond Oct 13, 2023

edleeks87 Oct 17, 2023

nineteen88 Oct 12, 2023

robinzigmond Oct 13, 2023

nineteen88 Oct 12, 2023

robinzigmond Oct 13, 2023

nineteen88 Oct 12, 2023

nineteen88 Oct 12, 2023

robinzigmond Oct 13, 2023

nineteen88 Oct 12, 2023

robinzigmond Oct 13, 2023

robinzigmond Oct 16, 2023

harpalsingh commented Nov 10, 2023

carbonci commented Dec 13, 2023

	Note the use of a `ref` to store the `FileReader` object - this is not essential but avoids having to create a new such object more than once in the
	Note the use of a `ref` to store the `FileReader` object - this is not essential but avoids having to create a new object more than once in the

	This example mocks an alternative approach where files added to the file input are immediately sent to the server, and the UI tracks the progress of this.
	This example mocks an alternative approach where files added to the file input are immediately sent to the server, and the UI tracks the progress.

	alternative endpoint for the upload progress, or use WebSocket messages sent from the serber to update progress.
	alternative endpoint for the upload progress, or use WebSocket messages sent from the server to update progress.


		import FileInput, { FileUploadStatusProps } from ".";

		describe("FileInput component", () => {

feat: add new file-input component #6355

feat: add new file-input component #6355

Conversation

robinzigmond commented Oct 9, 2023 • edited by divyajindel Loading

Proposed behaviour

Current behaviour

Checklist

QA

Additional context

Testing instructions

codesandbox-ci bot commented Oct 9, 2023 • edited Loading

edleeks87 left a comment

Choose a reason for hiding this comment

nineteen88 left a 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

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

harpalsingh commented Nov 10, 2023

carbonci commented Dec 13, 2023

robinzigmond commented Oct 9, 2023 •

edited by divyajindel

Loading

codesandbox-ci bot commented Oct 9, 2023 •

edited

Loading