Skip to content

fix(FileUpload): Improving events in the FileUpload#6139

Merged
tlabaj merged 18 commits intopatternfly:mainfrom
wojta:issue-5545-file-upload-events
Nov 9, 2021
Merged

fix(FileUpload): Improving events in the FileUpload#6139
tlabaj merged 18 commits intopatternfly:mainfrom
wojta:issue-5545-file-upload-events

Conversation

@wojta
Copy link
Member

@wojta wojta commented Aug 6, 2021

What: Closes #5545

What changes?

  • onChange is now split in more events, old event remain there for the compatibility
  • onTextChange - when text area changes (@mturley edit: renamed from onTextChanged)
  • onFileInputChange - called when internal <input> field emits its own onChange DOM event (@mturley edit: renamed from onInputChange)
  • onDataChange - when data is loaded in the component (@mturley edit: renamed from onDataChanged)
  • onClearClick - when clear button is clicked (@mturley edit: renamed from onClearClicked)
  • react-dropzone upgraded to the 10.2.2

@wojta wojta force-pushed the issue-5545-file-upload-events branch from 0e3e1e8 to ee40e03 Compare August 6, 2021 09:46
@wojta wojta requested a review from redallen August 6, 2021 09:51
@patternfly-build
Copy link
Collaborator

patternfly-build commented Aug 6, 2021

@nicolethoen
Copy link
Contributor

So far the new properties make sense to me! :)

@wojta
Copy link
Member Author

wojta commented Aug 11, 2021

I agreed with @christianvogt on adding onInputChanged that will do same as the input's onChange. No I fully understand the goal of the task is to provide replacement for HTML input type=file , but keeping the same implementation approach.

@mturley
Copy link
Collaborator

mturley commented Aug 12, 2021

@wojta do you have an opinion on #3966 ? I think it might make sense to also remove the dropzoneProps and expose new props to meet the use cases we were solving by opening those props up. The consumer shouldn't necessarily need to know/care about Dropzone.

@wojta wojta force-pushed the issue-5545-file-upload-events branch from c2b3d63 to d106ffd Compare August 27, 2021 17:07
@wojta wojta force-pushed the issue-5545-file-upload-events branch from d6fc959 to cd9f6e9 Compare September 18, 2021 20:31
@wojta wojta changed the title [WIP] fix(FileUpload): Improving events in the FileUpload fix(FileUpload): Improving events in the FileUpload Sep 19, 2021
@wojta wojta requested a review from redallen September 19, 2021 05:31
redallen
redallen previously approved these changes Sep 20, 2021
@redallen
Copy link
Contributor

Thanks for your perseverance @wojta !

@wojta wojta force-pushed the issue-5545-file-upload-events branch 2 times, most recently from 6519006 to 2db9839 Compare October 2, 2021 19:03
@tlabaj tlabaj modified the milestone: Prioritized Backlog Oct 7, 2021
Copy link
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

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

This largely looks great, but I have a couple of thoughts here. @redallen @nicolethoen do you think it is worth holding this PR up for these changes? I'd be happy to push a commit to this branch if you agree with the feedback since @vojtechszocs may be unavailable.

Looking at how this relates to #3966, I think we can follow up on that in a separate PR since it'll take consideration on how to deprecate dropzoneProps without removing it. I just think maybe the tweaks here will prevent merging some minor tech debt.

/** Value to be shown in the read-only filename field. */
filename?: string;
/** A callback for when the file contents change. */
/** *(deprecated)* A callback for when the file contents change. Please use rather the individual events. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think since this is still a beta component, maybe we can just remove this rather than deprecate it?

Edit: ah... I missed when we promoted this from beta. I should have followed up on #3966 sooner 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

can we change the wording here a bit. The Please use rather the individual events is a bit confusing.

Copy link
Collaborator

@mturley mturley Nov 9, 2021

Choose a reason for hiding this comment

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

How about "A callback for when the file contents change. Please instead use onFileInputChange, onTextChange, onDataChange, onClearClick individually."?

I'm also realizing I didn't propagate some of these changes to the examples markdown. Fixing now as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tlabaj I pushed fixes for these and rebased.

@redallen
Copy link
Contributor

I think it's worth holding up and it's okay for you to push to this branch @mturley .

@mturley
Copy link
Collaborator

mturley commented Oct 11, 2021

@redallen I pushed commits to resolve my comments. I also renamed the onTextChanged and onDataChanged to onTextChange and onDataChange respectively, to be consistent with the onFileInputChange and onChange naming tense.

@mturley
Copy link
Collaborator

mturley commented Oct 11, 2021

Also onClearClicked -> onClearClick, missed that one.

@nicolethoen
Copy link
Contributor

@kmcfaul @tlabaj this is ready for a final review

@nicolethoen nicolethoen requested a review from tlabaj November 4, 2021 16:59
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

Looks great. Jut a couple of small nits

/** Value to be shown in the read-only filename field. */
filename?: string;
/** A callback for when the file contents change. */
/** *(deprecated)* A callback for when the file contents change. Please use rather the individual events. */
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change the wording here a bit. The Please use rather the individual events is a bit confusing.

@mturley mturley force-pushed the issue-5545-file-upload-events branch from 4490637 to fc0fa0f Compare November 9, 2021 16:40
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.

FileUpload does not fire change events from the file input element

6 participants