Required meta fields UI #3285

arturi · 2021-11-01T09:48:40Z

Better UI for #2896

Added uppy.#checkRequiredMetaFieldsOnFile to check if the error has been fixed after a single file was edited and update the UI.

Mockups from @nqst:

…lds after editing

aduh95 · 2021-11-01T18:26:45Z

packages/@uppy/core/src/Uppy.js

-class AggregateRestrictionError extends AggregateError {
-  constructor (...args) {
-    super(...args)
-    this.isRestriction = true
-  }
-}


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

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.

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

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

I suggest to revert https://github.com/transloadit/uppy/pull/3285/files#diff-e522340b6dabc4b8f14b055b742c5c1adca74d15a06f06467956a59b7e106cc2L580-R589 and keep using it 😅 EDIT: see https://github.com/transloadit/uppy/pull/3285/files#r741085087

aduh95 · 2021-11-01T18:28:24Z

packages/@uppy/core/src/Uppy.js

+   *
+   */
+  #checkRequiredMetaFields (files) {
+    const errors = Object.keys(files).map((fileID) => {


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) => {

Thanks for the flatMap tip!

aduh95 · 2021-11-01T18:30:32Z

packages/@uppy/dashboard/src/components/FileItem/MetaErrorMessage.js

+  const metaFieldsString = missingRequiredMetaFields.map(field => (
+    field.charAt(0).toUpperCase() + field.slice(1)
+  )).join(', ')


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

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

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?

Murderlon · 2021-11-02T10:08:47Z

packages/@uppy/dashboard/src/components/FileItem/MetaErrorMessage.js

+  const metaFieldsString = missingRequiredMetaFields.map(field => (
+    field.charAt(0).toUpperCase() + field.slice(1)
+  )).join(', ')


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?

Murderlon · 2021-11-02T10:11:19Z

packages/@uppy/core/src/Uppy.js

+        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 })


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

No, showInformer: false.

aduh95 · 2021-11-02T14:00:37Z

packages/@uppy/core/src/Uppy.js


    if (errors.length) {
-      throw new AggregateRestrictionError(`${this.i18n('missingRequiredMetaField')}`, errors)
+      throw new RestrictionError(`${this.i18n('missingRequiredMetaField')}`)


Suggested change

throw new RestrictionError(`${this.i18n('missingRequiredMetaField')}`)

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

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

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.

* main: @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079) Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297) Add showRecordingLength to webcam types (#3303) Required meta fields UI (#3285) Update BACKLOG.md Release uppy@2.2.2 @uppy/provider-views@2.0.5

* main: Remove references of incorrect `options` argument for `companion.socket` (#3307) Upgrade linting to 2.0.0-0 (#3280) Release uppy@2.1.2, @uppy/robodog@2.1.3 @uppy/core@2.1.2 meta: update version references in docs Release @uppy/robodog@2.1.2 @uppy/unsplash@2.0.3 @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079) Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297) Add showRecordingLength to webcam types (#3303) Required meta fields UI (#3285) Update BACKLOG.md Release uppy@2.2.2 @uppy/provider-views@2.0.5 fix: @uppy/utils - `getFileType()` always returns a string (#3294) Remove yarn file @uppy/companion: fix Dockerfiles (#3291) @uppy/companion: use Node.js 16.x for Dockerfile (#3289) if an item is checked, it can’t be disabled (#3288)

* main: (30 commits) Refactor locale scripts & generate types and docs (#3276) Remove references of incorrect `options` argument for `companion.socket` (#3307) Upgrade linting to 2.0.0-0 (#3280) Release uppy@2.1.2, @uppy/robodog@2.1.3 @uppy/core@2.1.2 meta: update version references in docs Release @uppy/robodog@2.1.2 @uppy/unsplash@2.0.3 @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079) Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297) Add showRecordingLength to webcam types (#3303) Required meta fields UI (#3285) Update BACKLOG.md Release uppy@2.2.2 @uppy/provider-views@2.0.5 fix: @uppy/utils - `getFileType()` always returns a string (#3294) Remove yarn file @uppy/companion: fix Dockerfiles (#3291) @uppy/companion: use Node.js 16.x for Dockerfile (#3289) if an item is checked, it can’t be disabled (#3288) ...

* main: (30 commits) Refactor locale scripts & generate types and docs (transloadit#3276) Remove references of incorrect `options` argument for `companion.socket` (transloadit#3307) Upgrade linting to 2.0.0-0 (transloadit#3280) Release uppy@2.1.2, @uppy/robodog@2.1.3 @uppy/core@2.1.2 meta: update version references in docs Release @uppy/robodog@2.1.2 @uppy/unsplash@2.0.3 @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (transloadit#3079) Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (transloadit#3297) Add showRecordingLength to webcam types (transloadit#3303) Required meta fields UI (transloadit#3285) Update BACKLOG.md Release uppy@2.2.2 @uppy/provider-views@2.0.5 fix: @uppy/utils - `getFileType()` always returns a string (transloadit#3294) Remove yarn file @uppy/companion: fix Dockerfiles (transloadit#3291) @uppy/companion: use Node.js 16.x for Dockerfile (transloadit#3289) if an item is checked, it can’t be disabled (transloadit#3288) ...

arturi added 2 commits November 1, 2021 09:22

add checkRequiredMetaFieldsOnFile for checking single file's meta fie…

e0ecab1

…lds after editing

add MetaErrorMessage component, mobile and desktop styles

363ce4d

arturi requested review from aduh95 and Murderlon and removed request for aduh95 November 1, 2021 09:48

arturi marked this pull request as draft November 1, 2021 10:34

arturi requested a review from aduh95 November 1, 2021 10:34

aduh95 reviewed Nov 1, 2021

View reviewed changes

Murderlon reviewed Nov 2, 2021

View reviewed changes

aduh95 reviewed Nov 2, 2021

View reviewed changes

use AggregateRestrictionError again

a0143f9

Murderlon mentioned this pull request Nov 4, 2021

Status bar error state improvements #3299

Merged

arturi added 4 commits November 4, 2021 14:33

metaFieldIdToName — use names of meta fields instead of id

a5ad997

Update MetaErrorMessage.js

2a152d8

desktop style tweaks

b64717f

update jest snapshot

35c7cc7

arturi marked this pull request as ready for review November 5, 2021 00:55

Murderlon approved these changes Nov 5, 2021

View reviewed changes

arturi merged commit 6e4884f into main Nov 5, 2021

arturi deleted the required-meta-ui branch November 5, 2021 15:00

arturi mentioned this pull request Nov 5, 2021

@uppy/core: fix required metafields UX #3121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Required meta fields UI #3285

Required meta fields UI #3285

arturi commented Nov 1, 2021 •

edited

aduh95 Nov 1, 2021

arturi Nov 1, 2021

aduh95 Nov 2, 2021

arturi Nov 2, 2021

aduh95 Nov 2, 2021 •

edited

aduh95 Nov 1, 2021

arturi Nov 5, 2021

aduh95 Nov 1, 2021

arturi Nov 1, 2021

Murderlon Nov 2, 2021

Murderlon Nov 2, 2021

Murderlon Nov 2, 2021

arturi Nov 2, 2021

aduh95 Nov 2, 2021

arturi Nov 2, 2021

aduh95 Nov 3, 2021

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

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

Required meta fields UI #3285

Required meta fields UI #3285

Conversation

arturi commented Nov 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 Nov 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturi commented Nov 1, 2021 •

edited

aduh95 Nov 2, 2021 •

edited