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(file-uploader): Prop to turn off or on images in Tracker #2988

Merged
merged 4 commits into from
Nov 17, 2022

Conversation

ErikCH
Copy link
Contributor

@ErikCH ErikCH commented Nov 15, 2022

Description of changes

This new prop allows customers to turn on or off images to display in the tracker component

Issue #, if available

Asana

Description of how you validated changes

Add test

Checklist

  • PR description included
  • yarn test passes
  • Tests are updated
  • No side effects or sideEffects field updated
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ErikCH ErikCH requested a review from a team as a code owner November 15, 2022 17:45
@changeset-bot
Copy link

changeset-bot bot commented Nov 15, 2022

⚠️ No Changeset found

Latest commit: 6b8f129

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

@@ -23,6 +23,7 @@ export function FileUploader({
multiple = true,
onError,
onSuccess,
showImages = true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defaults to true

Comment on lines 361 to 370
errorMessage={status?.fileErrors}
file={status.file}
hasImage={status.file?.type.startsWith('image/')}
url={URL.createObjectURL(status.file)}
fileState={status?.fileState}
hasImage={status.file?.type.startsWith('image/') && showImages}
key={index}
onChange={onNameChange(index)}
name={status.name}
onCancel={onFileCancel(index)}
onCancelEdit={onCancelEdit(index)}
onChange={onNameChange(index)}
onDelete={onDelete}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I resorted these alphabetically

Comment on lines 24 to 30
file,
fileState,
hasImage,
url,
name,
onCancel,
onCancelEdit,
onPause,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resorted Alphabetically

Comment on lines +72 to +74
onCancelEdit?: () => void;
onChange: (event: React.ChangeEvent<HTMLInputElement>) => void;
onDelete?: () => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resorted alphabetically too , that's the only change here

hasImage={status.file?.type.startsWith('image/')}
url={URL.createObjectURL(status.file)}
fileState={status?.fileState}
hasImage={status.file?.type.startsWith('image/') && showImages}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This the change I added. So now it has to have an image and showImages has to be true.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to change this logic a bit. I was thinking this prop would show or hide both the image and file icon depending if it is true or false. If showImages is true, there will always be either an image or icon. If showImages is false, no image or icon will appear. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! @dbanksdesign I can make it so it doesn't show the icon either!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in cf8a443

@ErikCH ErikCH temporarily deployed to ci November 15, 2022 18:05 Inactive
@ErikCH ErikCH temporarily deployed to ci November 15, 2022 18:05 Inactive
@ErikCH ErikCH temporarily deployed to ci November 15, 2022 18:05 Inactive
@ErikCH ErikCH temporarily deployed to ci November 15, 2022 18:05 Inactive
Copy link
Contributor

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

Looks good, just one clarification

hasImage={status.file?.type.startsWith('image/')}
url={URL.createObjectURL(status.file)}
fileState={status?.fileState}
hasImage={status.file?.type.startsWith('image/') && showImages}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to change this logic a bit. I was thinking this prop would show or hide both the image and file icon depending if it is true or false. If showImages is true, there will always be either an image or icon. If showImages is false, no image or icon will appear. Does that make sense?

@ErikCH ErikCH requested a review from dbanksdesign November 15, 2022 21:34
@ErikCH ErikCH temporarily deployed to ci November 15, 2022 21:53 Inactive
@ErikCH ErikCH temporarily deployed to ci November 15, 2022 21:53 Inactive
@ErikCH ErikCH temporarily deployed to ci November 15, 2022 21:53 Inactive
@ErikCH ErikCH temporarily deployed to ci November 15, 2022 21:53 Inactive
Copy link
Contributor

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

One non-blocking nit. LGTM!

Comment on lines 139 to 141
) : (
''
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
) : (
''
)}
) : null}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! I'll fix it really quickly

@ErikCH ErikCH temporarily deployed to ci November 16, 2022 18:02 Inactive
@ErikCH ErikCH temporarily deployed to ci November 16, 2022 18:02 Inactive
@ErikCH ErikCH temporarily deployed to ci November 16, 2022 18:02 Inactive
@ErikCH ErikCH temporarily deployed to ci November 16, 2022 18:02 Inactive
@ErikCH ErikCH temporarily deployed to ci November 17, 2022 16:48 Inactive
@ErikCH ErikCH temporarily deployed to ci November 17, 2022 16:48 Inactive
@ErikCH ErikCH temporarily deployed to ci November 17, 2022 16:48 Inactive
@ErikCH ErikCH temporarily deployed to ci November 17, 2022 16:48 Inactive
@ErikCH ErikCH merged commit 55e3015 into fileuploader/main Nov 17, 2022
@ErikCH ErikCH deleted the fileuploader/turnoffpreview branch November 17, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants