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

docs(FileUpload): add API decisions #2010

Merged
merged 12 commits into from
Feb 28, 2024

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Feb 8, 2024

Pretty Markdown Doc

This PR addresses the decisions made for the FileUpload component API.

  • API Specification: Detailed the structure, functionality, and visual representation of the FileUpload component API.
  • Examples: Provided illustrative examples showcasing various use cases, including controlled and uncontrolled behaviors, single and multiple file selections.

Copy link

changeset-bot bot commented Feb 8, 2024

⚠️ No Changeset found

Latest commit: 306c303

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codesandbox-ci bot commented Feb 8, 2024

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 306c303:

Sandbox Source
razorpay/blade: basic Configuration

Copy link
Contributor

github-actions bot commented Feb 8, 2024

✅ PR title follows Conventional Commits specification.

@snitin315 snitin315 marked this pull request as ready for review February 9, 2024 04:42
@snitin315 snitin315 added the Review - L1 First level of review label Feb 9, 2024
@snitin315 snitin315 force-pushed the docs/file-upload-api-descisions branch from 9323a6d to e7d0e60 Compare February 13, 2024 07:23

type BladeFileList = BladeFile[];

type FileUploadCommonProps = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What callback would the user get when an upload is cancelled midway?

Copy link
Member Author

Choose a reason for hiding this comment

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

As per the current API, onRemove will be triggered when CloseIcon or TrashIcon is clicked. But users may want to use different logic so we should introduce a newonCancel prop. Otherwise they would need to rely on file.status which may not be ideal.

Screenshot 2024-02-20 at 2 44 56 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

onRemove theek hai na?

So upload can get cancelled 2 ways

  1. User cancels it - we call onRemove because user can cancel it only by clicking the X button
  2. Internet/Server Failure - what will we call here?

Copy link
Member Author

@snitin315 snitin315 Feb 23, 2024

Choose a reason for hiding this comment

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

@chaitanyadeorukhkar In case of server failure, the consumers will need to set file.status to error. We can cancel/remove via trashicon/closeicon button only. We don't have control over the submit/upload logic so consumers can control their server failures.

Screen.Recording.2024-02-23.at.4.24.42.PM.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

onRemove theek hai na?

So we trigger onRemove when either CloseIcon Or TrashIcon is clicked, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

try {
  // fetch api call
} catch (err) {
 // file.status = "error"
// file.errorText = error.message
}

After this the file is displayed in error state:

Screenshot 2024-02-27 at 11 36 05 AM

Now clicking the close icon triggers onCancel. Also trigger when clicking the close icon when uploading files:
Screenshot 2024-02-27 at 11 37 30 AM

@saurabhdaware

Copy link
Member

Choose a reason for hiding this comment

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

no I mean is it us who call API internally in our component? or do consumers call it?

Copy link
Member Author

Choose a reason for hiding this comment

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

no I mean is it us who call API internally in our component? or do consumers call it?

@saurabhdaware Consumers have control of the submit logic, check examples I've added in this doc for more info

Copy link
Member

@saurabhdaware saurabhdaware Feb 27, 2024

Choose a reason for hiding this comment

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

so clicking on close icon always calls onCancel right? should we call it onCloseButtonClick, onDismiss in that case similar to our other components?

Because to me, cancel implies that uploading got cancelled but in case where we show error and show close icon, we're not exactly cancelling the upload request no? upload request just failed the validation and the callback was called AFTER the consumer dismissed the error
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, onDismiss is more suitable.

@snitin315 snitin315 changed the base branch from master to file-upload-integration February 28, 2024 06:04
@snitin315 snitin315 merged commit b34bd74 into file-upload-integration Feb 28, 2024
16 checks passed
@snitin315 snitin315 deleted the docs/file-upload-api-descisions branch February 28, 2024 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Decisions Review - L1 First level of review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants