refactor: file picker component #1072
Conversation
superpower1
requested review from
lightbringer1991,
adityasabnis,
lteacher and
devharris7
| value={value || fileName} | ||
| title={value || fileName} | ||
| onClick={clickFile} | ||
| /> |
There was a problem hiding this comment.
@superpower1 @minhe86 can you put dts in this input as well, thank you.
There was a problem hiding this comment.
something like data-test-selector={`${dts}-form-control`}, if dts exists.
There was a problem hiding this comment.
I will put dts to the wrapper div instead
superpower1
force-pushed
the
file-picker
branch
from
239d615 to
8f1f580
Compare
Fixed. |
| /** | ||
| * data-test-selector of the rich text editor | ||
| */ | ||
| dts: PropTypes.string, |
There was a problem hiding this comment.
Sneak in a change to add dts to RichTextEditor.
superpower1
force-pushed
the
file-picker
branch
from
8f1f580 to
944d286
Compare
| /** | ||
| * function called when the file name changes | ||
| */ | ||
| onChange: PropTypes.func, |
There was a problem hiding this comment.
I can't exactly recollect the discussion, however, didn't we decide to cut down the handler functions.
do we need onSelect, onClick, onChange, onRemove for this picker
There was a problem hiding this comment.
I think we were planning to retain the onSelect for scenarios where the value is handled by the component
| const file = event.target.files[0]; | ||
| setIsFileSelected(true); | ||
| if (_.isFunction(onChange)) { | ||
| onChange(file.name); |
There was a problem hiding this comment.
I think we should probably provide the actual file for the onChange instead of just the name and allow the external component to put the file's name into the value. In those scenarios they would also not need to use onSelect since they have the power to do whatever and they also have the file itself to manage rather than it being hidden in the component.
| if (_.isFunction(onChange)) { | ||
| onChange(''); | ||
| } | ||
| setFileName(''); | ||
|
|
||
| if (_.isFunction(onRemove)) { | ||
| onRemove(); |
There was a problem hiding this comment.
Not quite sure about this. Triggering multiple handlers here seems to be a little bit confusing to me.
IMO should only keep either onChange('') or onRemove(), otherwise there will be multiple ways of doing the same thing.
the usage should be either
// handle remove with onChange('')
handleChange = (value) => {
if(value==='') {/* handle remove */}
}or
// setValue('') with onRemove()
handleRemove = () => setValue('')
Description
onClickprop for click event on file nameonChangeprop for change event on file namevalueprop to make the file name controllableaui--filepicker-component(Breaking Change)dtsprop to Rich Text Editor component (Requested by E2E)Does this PR introduce a breaking change?
Manual testing step?
Screenshots (if appropriate):