Skip to content

Introduce custom design of FileInputField (#244)#601

Merged
adamkudrna merged 2 commits into
masterfrom
feature/244-custom-file-input-field
Apr 11, 2025
Merged

Introduce custom design of FileInputField (#244)#601
adamkudrna merged 2 commits into
masterfrom
feature/244-custom-file-input-field

Conversation

@adamkudrna

Copy link
Copy Markdown
Member
obrazek obrazek obrazek

Closes #244.

@adamkudrna
adamkudrna force-pushed the feature/244-custom-file-input-field branch 3 times, most recently from 3137de8 to 17e8a64 Compare March 3, 2025 22:32
Comment thread src/components/FileInputField/FileInputField.jsx Outdated
Comment thread src/components/FileInputField/__tests__/FileInputField.test.jsx Outdated
Comment thread src/components/FileInputField/__tests__/FileInputField.test.jsx
Comment thread src/components/FileInputField/FileInputField.jsx Outdated
Comment thread src/components/FileInputField/FileInputField.jsx Outdated
Comment thread src/styles/theme/_form-fields.scss
Comment thread src/translations/en.js
Comment thread src/components/FileInputField/__tests__/FileInputField.test.jsx Outdated
Comment thread src/components/FileInputField/FileInputField.jsx
Comment thread src/components/FileInputField/FileInputField.jsx
@adamkudrna
adamkudrna force-pushed the feature/244-custom-file-input-field branch from 67dd040 to c779f4f Compare March 5, 2025 22:09

@bedrich-schindler bedrich-schindler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During our call with @mbohal, we agreed on several things:

  • Change of onDragOver is described here: #601 (comment)
  • Due to complexity, we do not want to use synthetic events but rather custom prop that would cover input change and file drop at one place, something like onFilesChanged and document it. This is missing in the documentation yet. Due to its complexity, this can't be omitted.
  • We do not want to make drag props available as we do not see benefits now and while do not want to use synthetic event. onChange, it would follow the same approach.
  • We are not sure about transferProps. Currently, input is hidden and logic is provided through button and droppable area. While we do not want to provide traditional synthetic event onChange and input is hidden, are transfer props placed on correct element? Is input where all aria attributes should be placed? We frankly do not know yet.

@mbohal

mbohal commented Mar 10, 2025

Copy link
Copy Markdown
Contributor

We are not sure about transferProps.

We want transferProps to point on the <input />.

@adamkudrna

Copy link
Copy Markdown
Member Author
  • Due to complexity, we do not want to use synthetic events but rather custom prop that would cover input change and file drop at one place, something like onFilesChanged and document it. This is missing in the documentation yet. Due to its complexity, this can't be omitted.

  • We do not want to make drag props available as we do not see benefits now and while do not want to use synthetic event. onChange, it would follow the same approach.

@bedrich-schindler @mbohal I admit I need a guidance regarding this change. Could you drop a suggestion somewhere in the code so I can fully understand it and do the rest of the work? 🙏🏻

@bedrich-schindler

Copy link
Copy Markdown
Contributor

Due to complexity, we do not want to use synthetic events but rather custom prop that would cover input change and file drop at one place, something like onFilesChanged and document it. This is missing in the documentation yet. Due to its complexity, this can't be omitted.

This means that prop onFilesChanged should be introduced and then on input type="file", do something like:

<input onChange={(e) => {
  onFilesChanged(e.target.files, e);
}}

and on droppable label (I guess):

<label onDrop={(e) => {
  onFilesChanged(dataTransfer.files, e);
}}

Maybe prevent default actions if needed. And document this.

@bedrich-schindler

bedrich-schindler commented Mar 19, 2025

Copy link
Copy Markdown
Contributor

We do not want to make drag props available as we do not see benefits now and while do not want to use synthetic event. onChange, it would follow the same approach.

This was request from @mbohal. You can for example use spread to omit all drag/drop events from the transfer props, so even if defined, it never gets into component. This should be documented as well.

@mbohal

mbohal commented Mar 19, 2025

Copy link
Copy Markdown
Contributor

You can for example use spread to omit all drag/drop events from the transfer props, so even if defined, it never gets into component.

Since the drop related event handlers must be specified on the root <div> and the transferProps are passed to the <input /> element I dont think we need to filter anything. Passing them via transferProps will not work anyway.

So, for example, I think that in dropHandler we can just remove calling the onDrop function.
The final code would thus look like this:

  const handleDrop = (event) => {
    event.preventDefault();
    handleFileChange(event.dataTransfer.files);
    setIsDragging(false);
}

I hope I answered. If not, just give me a call so we can get this out of the way.

@bedrich-schindler

Copy link
Copy Markdown
Contributor

I haven't checked the code itself yet, but there is still missing information in the docs.

As this is more complex input with custom event handlers, I would like to have at least some note or section about this. The only documentation of onFilesChanged is this:

image

There are no function parameters visible nor the example is present.

Currently, section Forwarding HTML Attributes is telling lies as not all events are passed.

I would like to:

  • Add example how the event should be handled and tell user that this is abstraction for onChange and onDrop.
  • Tell users that some events are not available and should not be used.

@bedrich-schindler

Copy link
Copy Markdown
Contributor

If I have:

<FileInputField
label="Attachment"
onFilesChanged={(e) => {
  console.log(e);
}}
/>

onFilesChanged is not called when file is dropped, only label changes.

@bedrich-schindler bedrich-schindler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to provide comment for "Request changes", but the issues are already top comments.

Comment thread src/components/FileInputField/FileInputField.jsx Outdated
Comment thread src/components/FileInputField/FileInputField.jsx Outdated
Comment thread src/components/FileInputField/FileInputField.jsx Outdated
Comment thread src/components/FileInputField/README.md
Comment thread src/components/FileInputField/FileInputField.jsx
Comment thread src/components/FileInputField/FileInputField.jsx Outdated
Comment thread src/components/FileInputField/FileInputField.module.scss Outdated
Comment thread src/components/FileInputField/FileInputField.jsx Outdated
@adamkudrna
adamkudrna force-pushed the feature/244-custom-file-input-field branch from 7e8eeaa to 7addc9a Compare April 11, 2025 17:54
@adamkudrna

Copy link
Copy Markdown
Member Author

FYI @mbohal @bedrich-schindler, I made the id mandatory so the input is paired with its label. That also allowed me to re-enable the previously disabled disabledPropTest.

@adamkudrna

Copy link
Copy Markdown
Member Author

Approved on a call last week, merging.

@adamkudrna
adamkudrna merged commit cecd54d into master Apr 11, 2025
@adamkudrna
adamkudrna deleted the feature/244-custom-file-input-field branch April 11, 2025 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom FileInputField

3 participants