-
Notifications
You must be signed in to change notification settings - Fork 139
feat: add FileDropZone component #2487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Preview is ready. |
|
Visual Tests Report is ready. |
e93907d to
8173352
Compare
|
The state of the component is not reset with such a case: Screen.Recording.2025-11-12.at.17.59.06.mov |
| export type UseFileInputProps = { | ||
| onUpdate?: (files: File[]) => void; | ||
| onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void; | ||
| multiple?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The multiple prop was not initially added, as the hook gives the basic configuration for the native input. That is, if the consumer needs multiple, then you need to add it yourself, otherwise it turns out to be a strange proxy that passes the prop through and returns it in the same form without using it in any way in calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/hooks/useDropZone/useDropZone.ts
Outdated
| role: 'button', | ||
| }; | ||
|
|
||
| export interface DroppableProps extends Required<typeof DROP_ZONE_BASE_ATTRIBUTES> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually name props according to this logic <ComponentOrHookName> + details. This is necessary so that all types have their own name space + it improves DX when working with lib types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/hooks/useDropZone/README.md
Outdated
| ``` | ||
|
|
||
| The `useDropZone` hook provides props for an element to act as a drop zone and also gives access to the dragging-over state. | ||
| Additionally, the hook supports a more imperative approach: it can accept a `ref` for cases where you don't have direct access to the HTML element you want to make a drop zone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe the minimum usage examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/hooks/useDropZone/useDropZone.ts
Outdated
|
|
||
| export interface UseDropZoneState { | ||
| isDraggingOver: boolean; | ||
| getDroppableProps: () => DroppableProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be no reason to make this as function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function will scale better in case parameterization is needed: you can just add optional arguments instead of overloading the type and turning it into a union. In my view, the function is a better fit here.
| } | ||
|
|
||
| function typeMatchesPattern(actualMimeType: string, expectedMimeTypePattern: string): boolean { | ||
| const actualMimeTypeParts = actualMimeType.split('/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we assuming that only files will be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t quite understand the question. MIME types allow specifying arbitrary data. For instance, in the Storybook example, text/plain is used, which enables you to drag and drop HTML elements. Moreover, this way you can add custom MIME types like application/x-my-custom-type to make sure that only permitted elements are drag-n-dropped —this can be useful, for example, for a Kanban board app.
| description?: string; | ||
| buttonText?: string; | ||
| icon?: IconData | null; | ||
| errorIcon?: IconData | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need more than one icon property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed to me that since the base icon and the “error” icon are semantically different, it makes sense to allow them to be customized separately. If we don't want to give the component user this flexibility in order to maintain visual consistency, that's not a problem—we can just remove this prop.
| import i18n from '../i18n'; | ||
|
|
||
| type FileDropZoneButtonProps = { | ||
| className?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that it would be convenient to have the children property so that explicitly using the button in the component layout, you can explicitly define the text in the button itself and not in its parent
buttonText is convenient to use as a parent зкщзукен if you don't explicitly use FileDropZoneButton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation was as follows. First, I wanted to eliminate redundancy in the component's interface and make the parent's props the single source of truth in this regard. This way, the cognitive load on the component user is reduced, as they no longer need to keep two sets of props with the same meaning in mind. Second, this approach allows for the encapsulation of the logic related to text handling, ensuring consistency between the displayed texts and the component's internal states—for example, the user of the component won't need to handle error messages or grammatical pluralization themselves if the multiple prop is provided.
More generally, the compound component pattern here was intended solely to allow arbitrary layouts of elements, while fully hiding their internal implementation and their relationships.
| import {useFileZoneContext} from '../FileDropZone.Provider'; | ||
|
|
||
| type FileDropZoneDescriptionProps = { | ||
| className?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same with the children property as in FileDropZoneButton
| import i18n from '../i18n'; | ||
|
|
||
| type FileDropZoneTitleProps = { | ||
| className?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same with the children property as in FileDropZoneButton
|
|
||
| export interface FileDropZoneProps extends Pick<BaseInputControlProps, 'validationState'> { | ||
| accept: UseDropZoneAccept; | ||
| onAdd: (files: File[]) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use the name onUpdate for such callbacks, this is a familiar pattern for users of our library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
8173352 to
b9fb7a6
Compare
|
Preview is ready. |
|
🎭 Component Tests Report is ready. |
Thank you for finding that! I’ll fix it. |
b9fb7a6 to
dd99072
Compare
Fixed the nesting counter consistency when transitioning between states. |
41621aa to
0cda828
Compare
No description provided.