Skip to content

fix(all): enable input type file #458

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

Merged
merged 7 commits into from
May 4, 2020
Merged

fix(all): enable input type file #458

merged 7 commits into from
May 4, 2020

Conversation

Hyperkid123
Copy link
Member

@Hyperkid123 Hyperkid123 commented Apr 27, 2020

closes #307

this will have to be backported to V1

To Do

  • tests
  • docs
    • validation example

@Hyperkid123 Hyperkid123 requested review from skateman and rvsia April 27, 2020 15:14
@skateman
Copy link
Member

Can't really say anything useful without an example usage piece of code 🤔

@Hyperkid123
Copy link
Member Author

@Hyperkid123 Hyperkid123 changed the title [WIP] fix(all): enable input type file fix(all): enable input type file Apr 28, 2020
@codecov
Copy link

codecov bot commented Apr 28, 2020

Codecov Report

Merging #458 into master will increase coverage by 0.00%.
The diff coverage is 92.30%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #458   +/-   ##
=======================================
  Coverage   89.70%   89.71%           
=======================================
  Files         138      138           
  Lines        2342     2354   +12     
  Branches      766      770    +4     
=======================================
+ Hits         2101     2112   +11     
- Misses        241      242    +1     
Impacted Files Coverage Δ
...pf4-component-mapper/src/files/dual-list-select.js 100.00% <ø> (ø)
...ges/react-form-renderer/src/files/use-field-api.js 91.42% <80.00%> (-0.88%) ⬇️
...ges/react-form-renderer/src/files/form-renderer.js 100.00% <100.00%> (ø)
.../react-form-renderer/src/files/renderer-context.js 100.00% <100.00%> (ø)
...m-renderer/src/form-renderer/enhanced-on-change.js 96.42% <100.00%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb848f0...f654c4d. Read the comment docs.


# File input

Files cannot be easilly uploaded in JSON payload. In order to upload files usigng the input type *file* you can follow these steps.
Copy link
Contributor

Choose a reason for hiding this comment

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

easilly > easily

usigng > using


## File onChange payload

In order to successfully store the file reference, you have either use an input of type file or use an object with the following shape in your on change function ([visit MDN docs for more info on file upload](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file)).
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe: 'use an input of "file" type'


## Getting file from state.

When submitting, you will have to construct the binary via FormData or encode the file to Base64, depending on your use case. Be aware that FormData cannot be sent in the JSON payload. Binaries are destroyed when serializing JSON. There will be a list of field names with the type file avaiable in the submit function arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

avaiable > available


When submitting, you will have to construct the binary via FormData or encode the file to Base64, depending on your use case. Be aware that FormData cannot be sent in the JSON payload. Binaries are destroyed when serializing JSON. There will be a list of field names with the type file avaiable in the submit function arguments.

The `formApi.fileInputs` is an array of field names with `type: file`. Be aware that if your filed name is in nested, you have to use the lodash like method of getting the value from state.
Copy link
Contributor

Choose a reason for hiding this comment

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

filed > field

@@ -76,6 +76,9 @@ const useFieldApi = ({ name, initializeOnMount, component, render, validate, ...
};

const fieldProps = useField(name, enhancedProps);
if (fieldProps.input.type === 'file' && typeof fieldProps.input.value === 'object') {
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 this modification should be moved to the return statement of this hook.

Copy link
Member Author

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

Choose a reason for hiding this comment

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

With this the input object is basically being modified at two different places. I would keep it at one place - it would be more consistent and readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

its not going to be pretty. But ok

@@ -170,6 +177,8 @@ const useFieldApi = ({ name, initializeOnMount, component, render, validate, ...
...(arrayValidator ? { arrayValidator } : {}),
input: {
...fieldProps.input,
value:
Copy link
Contributor

Choose a reason for hiding this comment

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

What about?
...(fieldProps.input.type === 'file' && typeof fieldProps.input.value === 'object' && {value: fieldProps.input.value.inputValue})

Copy link
Member Author

@Hyperkid123 Hyperkid123 Apr 28, 2020

Choose a reason for hiding this comment

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

that's how I originally wrote it but linter wanted it this way

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦‍♂️

Copy link
Contributor

@rvsia rvsia 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 to me 👍

@davellx
Copy link

davellx commented May 1, 2020

I was trying to make my own component for file inputs but couldn't go past the enhancedOnChange at the moment so I'm glad this PR exists, thank you !
Maybe It should be a good thing to plug some of the basic validation (required, length ?) or to explain in the docs that validation mut be done manually for future users.

@Hyperkid123
Copy link
Member Author

Hyperkid123 commented May 1, 2020

I was trying to make my own component for file inputs but couldn't go past the enhancedOnChange at the moment so I'm glad this PR exists, thank you !
Maybe It should be a good thing to plug some of the basic validation (required, length ?) or to explain in the docs that validation mut be done manually for future users.

@davellx The validation of input files is a bit tricky compared to other components. I think adding an example with a custom validator is a good idea. 👍

@Hyperkid123 Hyperkid123 changed the title fix(all): enable input type file [WIP] fix(all): enable input type file May 1, 2020
@Hyperkid123 Hyperkid123 changed the title [WIP] fix(all): enable input type file fix(all): enable input type file May 4, 2020
@github-actions github-actions bot added docs docs pull requests PF3 labels May 4, 2020
@github-actions github-actions bot added PF4 PF4 pull request renderer React form renderer PR labels May 4, 2020
@Hyperkid123
Copy link
Member Author

@davellx @rvsia I have added validation to the docs example. We can think about proper file validator in different PR. There are enough tools available to make the file upload work as it is and I don't want to block it too much.

@rvsia rvsia merged commit d705b19 into master May 4, 2020
@Hyperkid123
Copy link
Member Author

🎉 This PR is included in version 2.2.0 🎉

The release is available on

Demo can be found here!

@Hyperkid123 Hyperkid123 deleted the file-input branch May 15, 2020 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs docs pull requests PF3 PF4 PF4 pull request released renderer React form renderer PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PF3: File upload component
4 participants