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

Required meta fields UI #3285

Merged
merged 7 commits into from Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 32 additions & 17 deletions packages/@uppy/core/src/Uppy.js
Expand Up @@ -32,12 +32,6 @@ if (typeof AggregateError === 'undefined') {
}
}
}
class AggregateRestrictionError extends AggregateError {
constructor (...args) {
super(...args)
this.isRestriction = true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing this? I think we should keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it’s unused, how do you suggest we use it? We used to show an error info message with all the missing meta fields errors — but not anymore. I thought to log it maybe.

Copy link
Member

Choose a reason for hiding this comment

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

I think it can be quite useful for debugging, and in any case it's really cheap to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linter was complaining that it was unused, do you suggest we log with it?

Copy link
Member

@aduh95 aduh95 Nov 2, 2021

Choose a reason for hiding this comment

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


/**
* Uppy Core module.
Expand Down Expand Up @@ -557,27 +551,42 @@ class Uppy {
}

/**
* Check if requiredMetaField restriction is met before uploading.
* Check if requiredMetaField restriction is met for a specific file.
*
*/
#checkRequiredMetaFields (files) {
#checkRequiredMetaFieldsOnFile (file) {
const { requiredMetaFields } = this.opts.restrictions
const { hasOwnProperty } = Object.prototype

const errors = []
for (const fileID of Object.keys(files)) {
const file = this.getFile(fileID)
for (let i = 0; i < requiredMetaFields.length; i++) {
if (!hasOwnProperty.call(file.meta, requiredMetaFields[i]) || file.meta[requiredMetaFields[i]] === '') {
const err = new RestrictionError(`${this.i18n('missingRequiredMetaFieldOnFile', { fileName: file.name })}`)
errors.push(err)
this.#showOrLogErrorAndThrow(err, { file, showInformer: false, throwErr: false })
}
const missingFields = []
for (let i = 0; i < requiredMetaFields.length; i++) {
if (!hasOwnProperty.call(file.meta, requiredMetaFields[i]) || file.meta[requiredMetaFields[i]] === '') {
const err = new RestrictionError(`${this.i18n('missingRequiredMetaFieldOnFile', { fileName: file.name })}`)
errors.push(err)
missingFields.push(requiredMetaFields[i])
this.#showOrLogErrorAndThrow(err, { file, showInformer: false, throwErr: false })
Copy link
Member

Choose a reason for hiding this comment

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

are we still logging required fields in informer even with the new UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, showInformer: false.

}
}
this.setFileState(
file.id,
{ missingRequiredMetaFields: missingFields }
)
return errors
}

/**
* Check if requiredMetaField restriction is met before uploading.
*
*/
#checkRequiredMetaFields (files) {
const errors = Object.keys(files).map((fileID) => {
Copy link
Member

Choose a reason for hiding this comment

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

You need to flatten the result, otherwise errors.length === Object.keys(files).length.

Suggested change
const errors = Object.keys(files).map((fileID) => {
const errors = Object.keys(files).flatMap((fileID) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the flatMap tip!

const file = this.getFile(fileID)
return this.#checkRequiredMetaFieldsOnFile(file)
})

if (errors.length) {
throw new AggregateRestrictionError(`${this.i18n('missingRequiredMetaField')}`, errors)
throw new RestrictionError(`${this.i18n('missingRequiredMetaField')}`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new RestrictionError(`${this.i18n('missingRequiredMetaField')}`)
throw new AggregateRestrictionError(errors, `${this.i18n('missingRequiredMetaField')}`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is going to show that huge informer bubble to the users?

Copy link
Member

Choose a reason for hiding this comment

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

Not it should be exactly the same. The reason we saw a huge informer before because the arguments were written the other way around (so the errors array was used in place of the error message). If we put everything where it belongs, it should be fine.

}
}

Expand Down Expand Up @@ -1277,6 +1286,12 @@ class Uppy {
this.calculateTotalProgress()
})

this.on('dashboard:file-edit-complete', (file) => {
if (file) {
this.#checkRequiredMetaFieldsOnFile(file)
}
})

// show informer if offline
if (typeof window !== 'undefined' && window.addEventListener) {
window.addEventListener('online', this.#updateOnlineStatus)
Expand Down
32 changes: 19 additions & 13 deletions packages/@uppy/dashboard/src/components/FileItem/FileInfo/index.js
@@ -1,5 +1,6 @@
const { h, Fragment } = require('preact')
const prettierBytes = require('@transloadit/prettier-bytes')
const MetaErrorMessage = require('../MetaErrorMessage')
const truncateString = require('@uppy/utils/lib/truncateString')

const renderFileName = (props) => {
Expand Down Expand Up @@ -54,22 +55,22 @@ const renderAuthor = (props) => {
}

const renderFileSize = (props) => props.file.size && (
<div className="uppy-Dashboard-Item-statusSize">
{prettierBytes(props.file.size)}
</div>
<div className="uppy-Dashboard-Item-statusSize">
{prettierBytes(props.file.size)}
</div>
)

const ReSelectButton = (props) => props.file.isGhost && (
<span>
{' \u2022 '}
<button
className="uppy-u-reset uppy-c-btn uppy-Dashboard-Item-reSelect"
type="button"
onClick={props.toggleAddFilesPanel}
>
{props.i18n('reSelect')}
</button>
</span>
<span>
{' \u2022 '}
<button
className="uppy-u-reset uppy-c-btn uppy-Dashboard-Item-reSelect"
type="button"
onClick={props.toggleAddFilesPanel}
>
{props.i18n('reSelect')}
</button>
</span>
)

const ErrorButton = ({ file, onClick }) => {
Expand Down Expand Up @@ -107,6 +108,11 @@ module.exports = function FileInfo (props) {
onClick={() => alert(props.file.error)} // TODO: move to a custom alert implementation
/>
</div>
<MetaErrorMessage
file={props.file}
i18n={props.i18n}
toggleFileCard={props.toggleFileCard}
/>
</div>
)
}
Expand Up @@ -46,6 +46,7 @@
display: inline-block;
text-transform: uppercase;
vertical-align: bottom;
margin-bottom: 5px;
}

.uppy-Dashboard-Item-reSelect {
Expand All @@ -54,5 +55,47 @@
font-size: inherit;
font-family: inherit;
}
// ...uppy-Dashboard-Item-status|
// ...uppy-Dashboard-Item-fileInfo|

.uppy-Dashboard-Item-errorMessage {
font-size: 11px;
font-weight: 500;
line-height: 1.3;
color: darken($red, 15%);
background-color: lighten($red, 45%);
padding: 5px 6px;
}

.uppy-Dashboard-Item-errorMessageBtn {
text-decoration: underline;
cursor: pointer;
font-weight: 500;
}

// Error message desktop / large screen
.uppy-Dashboard-Item-preview .uppy-Dashboard-Item-errorMessage {
display: none;
border-top: 1px solid lighten($red, 35%);
padding: 6px 8px;
line-height: 1.4;

.uppy-size--md & {
display: block;
position: absolute;
bottom: 0;
left: 0;
right: 0;
}
}

// Error message mobile / small screen
.uppy-Dashboard-Item-fileInfo .uppy-Dashboard-Item-errorMessage {
display: inline-block;
position: static;
border: 1px solid lighten($red, 35%);
border-radius: 3px;

.uppy-size--md & {
display: none;
}
}

@@ -1,5 +1,6 @@
const { h } = require('preact')
const FilePreview = require('../../FilePreview')
const MetaErrorMessage = require('../MetaErrorMessage')
const getFileTypeIcon = require('../../../utils/getFileTypeIcon')

module.exports = function FilePreviewAndLink (props) {
Expand All @@ -19,11 +20,16 @@ module.exports = function FilePreviewAndLink (props) {
target="_blank"
aria-label={props.file.meta.name}
>
<span hidden>props.file.meta.name</span>
<span hidden>{props.file.meta.name}</span>
</a>
)
}
<FilePreview file={props.file} />
<MetaErrorMessage
file={props.file}
i18n={props.i18n}
toggleFileCard={props.toggleFileCard}
/>
</div>
)
}
@@ -0,0 +1,29 @@
const { h } = require('preact')

module.exports = function renderMissingMetaFieldsError (props) {
const { file, toggleFileCard, i18n } = props
const { missingRequiredMetaFields } = file
if (!missingRequiredMetaFields) {
return null
}
const metaFieldsString = missingRequiredMetaFields.map(field => (
field.charAt(0).toUpperCase() + field.slice(1)
)).join(', ')
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to get the label instead of the name here by any chance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only through Dashboard options, right? Which is not ideal, as in theory Core is independent of Dashboard and doesn’t know about it.

Copy link
Member

Choose a reason for hiding this comment

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

But as you are making the UI for the dashboard and not for core, would it be possible to match ids of requiredMetaFields to metaFields and then use the name property from the latter?


return (
<div className="uppy-Dashboard-Item-errorMessage">
{i18n('missingRequiredMetaFields', {
smart_count: missingRequiredMetaFields.length,
fields: metaFieldsString,
})}
{' '}
<button
type="button"
class="uppy-u-reset uppy-Dashboard-Item-errorMessageBtn"
onClick={() => toggleFileCard(true, file.id)}
>
{i18n('editFile')}
</button>
</div>
)
}
3 changes: 3 additions & 0 deletions packages/@uppy/dashboard/src/components/FileItem/index.js
Expand Up @@ -76,6 +76,8 @@ module.exports = class FileItem extends Component {
<FilePreviewAndLink
file={file}
showLinkToFileUploadResult={this.props.showLinkToFileUploadResult}
i18n={this.props.i18n}
toggleFileCard={this.props.toggleFileCard}
/>
<FileProgress
uppy={this.props.uppy}
Expand All @@ -101,6 +103,7 @@ module.exports = class FileItem extends Component {
containerWidth={this.props.containerWidth}
i18n={this.props.i18n}
toggleAddFilesPanel={this.props.toggleAddFilesPanel}
toggleFileCard={this.props.toggleFileCard}
/>
<Buttons
file={file}
Expand Down
4 changes: 4 additions & 0 deletions packages/@uppy/dashboard/src/index.js
Expand Up @@ -105,6 +105,10 @@ module.exports = class Dashboard extends UIPlugin {
sessionRestored: 'Session restored',
reSelect: 'Re-select',
poweredBy: 'Powered by %{uppy}',
missingRequiredMetaFields: {
0: 'Missing required meta field: %{fields}.',
1: 'Missing required meta fields: %{fields}.',
},
},
}

Expand Down
4 changes: 4 additions & 0 deletions packages/@uppy/locales/src/en_US.js
Expand Up @@ -81,6 +81,10 @@ en_US.strings = {
micDisabled: 'Microphone access denied by user',
missingRequiredMetaField: 'Missing required meta fields',
missingRequiredMetaFieldOnFile: 'Missing required meta fields in %{fileName}',
missingRequiredMetaFields: {
'0': 'Missing required meta field: %{fields}.',
'1': 'Missing required meta fields: %{fields}.',
},
myDevice: 'My Device',
noCameraDescription: 'In order to take pictures or record video, please connect a camera device',
noCameraTitle: 'Camera Not Available',
Expand Down