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

File picker component #50

Closed
wants to merge 61 commits into from
Closed

File picker component #50

wants to merge 61 commits into from

Conversation

roblevintennis
Copy link
Contributor

@roblevintennis roblevintennis commented Aug 10, 2019

Type of work: bug / component / site / accessibility / other

Deployment URL: https://mavenlink.github.io/design-system/$BRANCH

(Optional for outside contributions) Tracked work in Mavenlink:

Motivation

As we are fleshing out the Custom Forms proof-of-concept, we need an input which can attach a file to a form. While the reusability of this particular component is low, it will serve to bring the project to completion. Perhaps this component might actually be used in Mavenlink designs or perhaps this is a component that should live in the mavenlink-solutions/project-and-task-forms repository.

Related long-term MDS release @ #165775767

Resources: https://sketch.cloud/s/VKakr/a/DDWWg4

Acceptance Criteria

Change log entry

@roblevintennis roblevintennis self-assigned this Aug 10, 2019
… shorthand which we haven't configured and causes errors. So using 9.0.4 for the time being.
… and the native open dialog appears to work better and in a more straight forward way without trying to label click handling too. Turns out label click is still working in local env anyway.
…e file dialog. In this case do not overwrite the files state with empty
…library/react since RTL is used in bigmaven already and has advantages, especially since the file picker is an uncontrolled component essentially.
"react-ga": "^2.5.6",
"react-styleguidist": "^8.0.6",
"react-styleguidist": "9.0.4",
"react-test-renderer": "^16.9.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgraded some of these libraries to be compatible with React Hooks:
enzymejs/enzyme#1938

@@ -64,19 +65,20 @@
"jest": "^23.6.0",
"postcss-loader": "^3.0.0",
"postcss-preset-env": "^6.5.0",
"react": "^16.7.0",
"react-dom": "^16.7.0",
"react": "^16.9.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

React Hooks support started in 16.8 and also we might as well keep up (bigmaven is already using hooks for example!)

@@ -48,14 +48,15 @@
"@babel/polyfill": "^7.4.4",
"@babel/preset-env": "^7.2.3",
"@babel/preset-react": "^7.0.0",
"@testing-library/react": "^9.1.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kent Dodds popular testing framework:
https://testing-library.com/docs/react-testing-library/intro

We're using this in bigmaven already and it's got some things that make it easier to test uncontrolled components too.

…tion which returns a string containing the errors. It will use that to set the display errors. Refactors specs and markdown examples accordingly.
@roblevintennis
Copy link
Contributor Author

roblevintennis commented Aug 19, 2019

Error handling using a validator function prop added. Here's the example:

image

MLProjCreation

@roblevintennis
Copy link
Contributor Author

File Picker in Edge after adding some files:

VirtualBox_Custom Forms IDE Clone_20_08_2019_11_02_16

receiveFilesChanged testing that the callback was called with file list:

VirtualBox_Custom Forms IDE Clone_20_08_2019_11_10_37

Validator test. In this case uploading a file > 10mb:

VirtualBox_Custom Forms IDE Clone_20_08_2019_11_12_58

Same checks were done in both Edge and IE11

@@ -0,0 +1,14 @@
{
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 can define the overrides in the top-level eslintrc with a files regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 68ee789

};

const onDragLeave = () => {
setHighlight(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

We are missing setHighlight(true) in this PR.

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 e4404e6

if (files.length) {
return files.map((file) => {
return (
<section className={styles['file-list-button']} key={file.name}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's convert this list of <section>s into a list of <li>s

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: 07b8b2f

roblevintennis and others added 7 commits August 20, 2019 14:14
…stom forms env, I was getting errors like: Module not found: Error: Can't resolve '../icon/icon' and it looks like in other places we're supplying extension. So, trying that.
@roblevintennis
Copy link
Contributor Author

roblevintennis commented Aug 21, 2019

@juanca since you'll be taking this over, hopefully this is some good context...

The following screenshots show 1. validation errors are working for files > 10mb 2. our file list appears properly above the dropzone upon selecting files 2. our callbacks are both working (Both receiveFilesChanged={onFilesChanged} and validator={validateFileSize}, and we properly receive a DOM FileList, etc.

VirtualBox_Custom Forms IDE Clone_21_08_2019_16_06_25
VirtualBox_Custom Forms IDE Clone_21_08_2019_16_09_02
VirtualBox_Custom Forms IDE Clone_21_08_2019_16_10_20

What will need to be done next?

I believe, if I'm understanding correctly, that the code in ProjectAndTaskLoadForm/MLProjCreation.aspx.cs that I've commented out since the UserFileUpload.FileName resource was removed and no longer exists (thus it was causing Errors to leave the reference), is where the actual serialization and uploading of files happened. Somehow, we'll need to figure out how to reproduce that in the new world on form submissions.

Search for // Selected Attachment upload in post to find file mentioned above.

But I think this proves the component works as advertised and can live in the custom forms env successfully.

Here's the latest commit for the Project-and-Task-Forms POC these screenshots refer to.

@roblevintennis
Copy link
Contributor Author

Some notes for posterity on commit: 69af234

I tried to fix following error on the Icon component page:

Error: import or require() statements can be added only by editing a Markdown example file: ../../svgs/icon-caution-fill.svg

image

Looks like this was a recent issue for styleguidist and so I followed directions on the issue attempting to nuke the lock file and reinstall:
styleguidist/react-styleguidist#1278
webpack/webpack#8656 (comment)

But the error didn't go away.

So, I tried following the other suggestion to add acorn et al as dev dep:
styleguidist/react-styleguidist#1278 (comment)

yarn add --dev acorn acorn-jsx

And that worked and the error went away.

@juanca juanca closed this Feb 5, 2020
@juanca juanca deleted the file-picker-component branch October 29, 2020 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants