Carbon: Unused code in FileUploader

[emyarod]

Detailed description

while working on #1022 I noticed some divergence in the FileUploader from the vanilla components. Looking for some clarification on how to proceed

Describe in detail the issue you're having.

When uploading files, the React FileUploader has some logic that will modify the label text. If the user is uploading multiple files, it will change the label text from Add files to the number of files currently being uploaded, and if the user is uploading a single file, the label text will be changed to the name of the file being uploaded. A couple things about the relevant code though:

1. it doesn't seem to be present in the vanilla components
2. it is unreachable in the current React component

should I remove it completely or is this a desirable feature extension from the vanilla implementation?

Is this a feature request (new component, new icon), a bug, or a general issue?

general issue

Is this issue related to a specific component?

FileUploader

What did you expect to happen? What happened instead? What would you like to see changed?

Right now I am working around this issue while refactoring the component, but it might not even be worth keeping since it is different from the vanilla implementation. Would like some input on this matter though.

What version of the Carbon Design System are you using?

6.6.3

Additional information

Current Vanilla and React FileUploader behavior:

The following behavior is present within React FileUploader but currently unreachable. The idea seems to be that if the user is uploading multiple files, it will change the label text from Add files to the number of files currently being uploaded, and if the user is uploading a single file, the label text will be changed to the name of the file being uploaded. Don't know if I agree with this behavior but if it needs to be modified I can add in the revised behavior. Otherwise I can remove the unreachable code:

[asudoh]

Thanks @emyarod for reporting! I feel I’m kind of one the fence - The React one shows a nice UI wrt what is being uploaded, but loses a context of additional files can be uploaded. CC @carbon-design-system/design to see which one is more desirable.

[emyarod]

hi @asudoh I am leaning towards the current behavior found in the vanilla component (not changing the text within the button), but I am fine with either option. I just want more clarification on the direction we want to proceed with as I fix the dead code issue.

One question though: is the FileUploaderButton ever used by itself (without the FileUploader) or is it always a child of a FileUploader component? I am assuming that FileUploaderButton should not be used by itself since the vanilla Carbon documentation does not show this usage pattern.

Right now in the Carbon React storybook, the FileUploaderButton by itself will change its own label text to the file name after uploading. If the FileUploaderButton is not intended to be used as a standalone component, it can be converted to a stateless class component along with avoiding the soon-to-be-deprecated cWRP lifecycle method. I believe it still needs to remain a class and not a function though to maintain its refs

[tay-aitken]

The button text should not be changing, but stay persistent, no matter how many files the user uploads. If the user wants to add more files after the initial upload, there are now no cues for how to go it. That shouldn't have been built into the react version of the component.

[emyarod]

@tay-aitken thanks for the response. The React version currently doesn't change its button text, but it does have some logic that (if properly connected) would cause that behavior. Since it shouldn't have been added to the component though, I will go ahead and remove the dead code

[tay-aitken]

thank you! Appreciate you doing that!

[emyarod]

is there a sandbox example with mock interactions for uploading files? The vanilla documentation for <FileUploader> shows a bunch of GIFs but they are taken from the React component, and the CodePen example only shows the appearance of the component without any app logic.

Reading through the current implementation though, it seems some of the features shown in the GIFs are not ideally implemented. It looks like file upload status is applied to all files in the component as opposed to being bound to each file individually (i.e. every file in the component will share the same status regardless of how far along it is in the upload process, or what the individual outcomes are of each upload).

[asudoh]

Hi @emyarod not sure if your first question is referring to the vanilla code or the React code, but here’s the way we test status updates in our vanilla implementation: The FileUploader vanilla class has setState(state, selectedIndex) API (doc), and we grab FileUploader instance via FileUploader.components to call setState() on. FileUploader.components is a WeakMap keyed by the component's DOM element.

To the 2nd question, I think you are right that filenameStatus dictating the status of all <Filename> seems to be a bug.

[emyarod]

sorry @asudoh I mistook the vanilla API setState for React setState, that was my mistake. But my question still remains:

Just to get a better idea of how to use the current <FileUploader>, I want to know if there is an example of <FileUploader> sending mock data and getting a mock response. If not, I think I will try to write a small example myself to better understand how the component works before doing any more major refactoring like applying filenameStatus to each file rather than a blanket status for all files

[asudoh]

Thanks @emyarod for articulating your question further! I think the only thing I know is http://react.carbondesignsystem.com/?selectedKind=FileUploader&selectedStory=FileUploader&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel - It'd be nice if you have some ideas on involving/adding the story. Thanks!

[emyarod]

@asudoh so I guess it is currently left up to the user to provide their own logic for making HTTP requests. This is fine as it is, if we want to keep the current behavior of applying 1 upload status to the entire upload operation. But if we want to have individual upload status for each file in the operation, would it be worth implementing that logic within <FileUploader> itself in order to manage the status of each file upload internally?

[asudoh]

Thanks alot @emyarod for articulating that you were looking for per-file upload status in our React variant and my apologies for not catching that! As vanilla supports that feature, I believe React should do that, too. Great to have that if it can be done in a non-breaking way. Thanks!

[emyarod]

@asudoh as we discussed on Slack, I will begin with V2 of the <FileUploader> component and I will also backport as much as I can via the manageStatusPerFile prop in V1

[asudoh]

Thank you so much for doing this @emyarod! Just a note - manageStatusPerFile prop in V1 thing is only for contemplating starting with new vs. backward-compatible change. That said, once we see the former wins, we don’t do the latter (or vice versa). Thanks agina!

[stale[bot]]

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. Thanks for your contributions.

[carbon-bot]

Hi there! :wave: If you're wondering why this issue was moved, we're currently updating our repo structure so that every package is found in the same project.

This should not have any impact for you, but we wanted to give you a heads up in case you were wondering what is going on. If you have any questions, feel free to reach out to us on Slack or contact us at: [email protected]. Thanks!

[stale[bot]]

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

[emyarod]

superseded by #2288 #2916 #2917