Skip to content
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

feat: KDS-1055 | Dropzone component #3353

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jared-luke
Copy link
Contributor

Why

This is a work in progress.
Introducing the new Dropzone component.
It is leveraging a library called react-dropzone (link here).
The reason behind this component is to create a consistent experience for our customers and engineers when using a file uploader.

Note: the functionality of uploading a file is yet to be worked on. Below is a checklist of things that need to be done before this PR is ready for review.

Currently, there have been four storybook stories to demonstrate the different scenarios of the Dropzone component. These include and can be seen in the video below:

  • Default component
  • Adjusting the maxFileSize prop. Defaults to 40kb
  • Passing state back up to the parent component. This is useful for forms, buttons, and workflows
  • Custom tailwind styles being applied to the parent container

Design file: Found here

What

dropzone-progress.mp4

Checklist

A Basic list of to-do's that will be worked on over time:

  • Write a complete suite of tests
  • Write a complete set of storybook stories
  • Internationalisation
  • Accessibility review
  • Finalise the documentation for release
  • Responsiveness review with designers

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

✨ Here is your branch preview! ✨

Last updated for commit 53b7b8b: fix: breaking the selected file name to improve responsiveness

Comment on lines +17 to +28
if (href) {
return (
<a
href={href}
aria-label={text}
target={openNewTab ? "_blank" : "_self"}
className="text-blue-500 underline hover:underline"
>
{text}
</a>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this component need to be a link? In any case, I would suggest splitting this into it's own component (or remove?) rather than combine them, doing it this way makes for an ambiguous/complicated API since an anchor and a button consume different props. eg. how would you decide which to render if both href and onClick was provided?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's in case your error message needs a link, this should be handled by the consumer passing in a custom error message in the form of a ReactNode.

Comment on lines +90 to +99
className={classNames(
{
"rounded-default": true,
"border-w-default border-dashed border-gray-400": true,
"transition-colors": true,
"text-body text-center text-purple-800 transition-colors": true,
"w-100 mb-4": true,
"!border-solid !border-blue-500 !bg-blue-100": isDragActive,
}
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid the need for all the true by passing each string in as a param:

Suggested change
className={classNames(
{
"rounded-default": true,
"border-w-default border-dashed border-gray-400": true,
"transition-colors": true,
"text-body text-center text-purple-800 transition-colors": true,
"w-100 mb-4": true,
"!border-solid !border-blue-500 !bg-blue-100": isDragActive,
}
)}
className={classNames(
"rounded-default",
"border-w-default border-dashed border-gray-400",
"transition-colors",
"text-body text-center text-purple-800 transition-colors",
"w-100 mb-4",
isDragActive &&"!border-solid !border-blue-500 !bg-blue-100"
)}

Copy link
Contributor

Choose a reason for hiding this comment

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

Although, what would be your appetite for converting all your tailwind to vanilla CSS?

Comment on lines +73 to +84
/**
* Set the parent `enableFileUpload` state to allow the parent component to update its UI
* Note: this logic will change when we allow multiple files in the future
*/
useEffect(() => {
const hasAcceptedFile = acceptedFiles.length >= 1
if (hasAcceptedFile) {
setEnableFileUpload(true)
} else {
setEnableFileUpload(false)
}
}, [acceptedFiles])
Copy link
Contributor

Choose a reason for hiding this comment

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

This function isn't necessary since the consumer can pass something similar in to onDrop and control what they need to from the outside. eg.

const handleRequiredDropzone = ({ fileRejections }) => {
  acceptedFiles.length >= 1 && setEnableSubmit(true)
}

<Dropzone onDrop={handleRequiredDropzone} />

import { OnDragMessage } from "./sub-components/OnDragMessage/OnDragMessage"
import { SelectedFileMessage } from "./sub-components/SelectedFileMessage/SelectedFileMessage"

type DropzoneProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

For the API, we're thinking it's best to extend the library's type with some minor tweaks:

  • Add the OverrideClassName type (copy this into your component)
  • Omit the multiple type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are you thoughts on extending or Picking some of the InputHTMLAttributes as well. This can allow attributes like name when used inside of a form element being passed down to the dropzone component as well

Comment on lines +31 to +37
noClick: true,
noKeyboard: true,
multiple: false,
maxFiles: 1,
accept: {
"application/type": ACCEPTED_FILE_TYPES,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

These should all come from props, with defaults added at the prop level.

Comment on lines +23 to +24
export const DropzoneIcon = ({ isDragActive, fileName }: { isDragActive: boolean, fileName: string | null }): JSX.Element => {
if (fileName) return <AcceptedFileIcon />
Copy link
Contributor

Choose a reason for hiding this comment

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

Best to avoid passing in a string to be treated as a boolean, a better way would be to have a hasFile prop and leave it up to the consumer to define how to achieve that boolean.

Comment on lines +87 to +89
<div className={classNames(className)}>
<div
data-automation-id={DROPZONE_AUTOMATION_ID}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an outdated method of applying test id's as the react-testing-library opts to use data-test-id

Having said that we prefer to allow the consumers to pass any extra prop they want through to the component, so we would spread the rest of the props onto your top level wrapping div (once extracting all the props required for the dropzone config).

height={20}
width={20}
icon={acceptedFileSvgIcon}
title="Selected File"
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an interesting issue where after the file has been attached, focus is lost completely and the screen reader jumps back to the browser window. Ideally what we expect to happen is the focus to shift to this icon (or it's container) and read out this title to let the SR user know that what they have done is successful.

import React from "react"
import { Paragraph } from "@kaizen/typography"

export const OnDragMessage = (): JSX.Element => (
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be renamed to sound more like a component, as we use the "On" prefix for functions.

Comment on lines +3 to +5
import { Icon } from "@kaizen/component-library"
import selectedFileSvgIcon from "@kaizen/component-library/icons/file.icon.svg"
import acceptedFileSvgIcon from "@kaizen/component-library/icons/success.icon.svg"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use local paths for these

return `${parseFloat((bytes / Math.pow(k, i)).toFixed(dm))}${sizes[i]}`
}

interface GetKaizenDropzoneError
Copy link
Contributor

Choose a reason for hiding this comment

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

We're inside Kaizen already, no need to namespace these 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants