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

Add FileUploader widget #1540

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Add FileUploader widget #1540

wants to merge 35 commits into from

Conversation

msssk
Copy link
Contributor

@msssk msssk commented Aug 27, 2020

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit tests are included in the PR
  • For new widgets, an entry has been added to the .dojorc
  • For new widgets, theme.variant() is added to the root domnode
  • Any widget variant uses theme.compose like this
  • WidgetProperties are exported

Description: add FileUploader widget

Resolves #1129

@vercel
Copy link

vercel bot commented Aug 27, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

dojo.widgets – ./

🔍 Inspect: https://vercel.com/dojo/dojowidgets/cpraac4yf
✅ Preview: https://dojowidgets-git-1129-uploader-widget.dojo1.now.sh

widget-test-docs – ./

🔍 Inspect: https://vercel.com/dojo/widget-test-docs/j4ljxjlc3
✅ Preview: https://widget-test-docs-git-1129-uploader-widget.dojo1.now.sh

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #1540 (f9f3e19) into master (e3dc8f5) will decrease coverage by 0.22%.
The diff coverage is 83.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1540      +/-   ##
==========================================
- Coverage   90.15%   89.93%   -0.23%     
==========================================
  Files          94       98       +4     
  Lines        4846     5016     +170     
  Branches     1297     1353      +56     
==========================================
+ Hits         4369     4511     +142     
- Misses        236      248      +12     
- Partials      241      257      +16     
Impacted Files Coverage Δ
src/file-upload-input/index.tsx 80.26% <80.26%> (ø)
src/file-uploader/index.tsx 85.55% <85.55%> (ø)
src/file-upload-input/nls/FileUploadInput.ts 100.00% <100.00%> (ø)
src/file-uploader/nls/FileUploader.ts 100.00% <100.00%> (ø)

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 e3dc8f5...f9f3e19. Read the comment docs.

@msssk msssk mentioned this pull request Aug 27, 2020
@tomdye
Copy link
Member

tomdye commented Aug 27, 2020

I added comments to the issue re. the separation of the two widgets. I don't think this will need a child renderer at all as the file-list version of the file upload widget would be opinionated in it's appearance. The only child would likely be the label as per other widgets. You should be able to use composes in your css to utilise the existing button styles for each of the themes also.

@vercel vercel bot had a problem deploying to Preview – dojo.widgets August 28, 2020 04:39 Failure
@vercel vercel bot had a problem deploying to Preview – widget-test-docs August 28, 2020 04:39 Failure
@vercel vercel bot had a problem deploying to Preview – dojo.widgets September 1, 2020 17:06 Failure
@vercel vercel bot had a problem deploying to Preview – widget-test-docs September 1, 2020 17:06 Failure
@vercel vercel bot had a problem deploying to Preview – dojo.widgets September 1, 2020 21:28 Failure
@vercel vercel bot had a problem deploying to Preview – widget-test-docs September 1, 2020 21:28 Failure
@vercel vercel bot had a problem deploying to Preview – widget-test-docs September 4, 2020 04:39 Failure
@vercel vercel bot had a problem deploying to Preview – dojo.widgets September 4, 2020 04:39 Failure
@vercel vercel bot had a problem deploying to Preview – dojo.widgets September 8, 2020 23:18 Failure
@vercel vercel bot had a problem deploying to Preview – widget-test-docs September 8, 2020 23:18 Failure
@vercel vercel bot had a problem deploying to Preview – dojo.widgets September 9, 2020 04:39 Failure
@vercel vercel bot had a problem deploying to Preview – widget-test-docs September 9, 2020 04:39 Failure
@vercel vercel bot had a problem deploying to Preview – widget-test-docs September 10, 2020 04:07 Failure
@vercel vercel bot had a problem deploying to Preview – dojo.widgets September 10, 2020 04:07 Failure
}
}

function onClickButton() {
Copy link
Member

Choose a reason for hiding this comment

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

if we really need to do this then perhaps you should rename this function to clickNativeButton to make it clearer what it is doing.


export interface FileUploaderProperties extends FileUploadInputProperties {
/** Custom validator used to validate each file */
customValidator?: (file: File) => ValidationInfo | void;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this pattern, other widgets set the valid property when they want to control the validation and use the the onValue function to trigger said validation.

customValidator?: (file: File) => ValidationInfo | void;

/** The files to render in the widget (controlled scenario) */
files?: FileWithValidation[];
Copy link
Member

Choose a reason for hiding this comment

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

Seems odd that the validation is being mixed into the files for all of these callbacks. Would have expected onValue etc and files to only take / pass File[] params.

@bitpshr bitpshr mentioned this pull request May 7, 2021
7 tasks
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.

Uploader widget
2 participants