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 People to Info panel and Upload page #3083

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

paperboyo
Copy link
Contributor

@paperboyo paperboyo commented Jan 11, 2021

This follows the amazing #3048.

  • It adds People field on the Upload page in the similar, conditional, way as Copyright: it’s there only if the uploaded image already has peopleInImage field present (whether we want it conditional should be confirmed with PIcture Desk before this gets merged)
  • It adds People field to the Info panel

This should make editing People metadata more accessible, convenient, consistent and should raise the profile of this field.

This is the draft as I have hit the limit of my abilities: all having to do with the array nature of the data.

Current unresolved issues:

  • 1. Editing People on the Upload page doesn’t work (strangely, it works in the Info panel):
    400: List((/peopleInImage,List(JsonValidationError(List(error.expected.jsarray),WrappedArray()))))

  • 2. When only one person in one selected image is present in the field, Info panel shows it in square brackets:

    image
  • 3. Even for one image, if there are multiple people in the field, Info panel shows what’s usually shown for multiple images unreconcilable data:

    image
  • 4. The same as above is shown for multiple images having exactly the same one person (or the same collection of people) in their People fields

  • 5. When one (or more) image(s) have no People data, Unknown (click ✎ to… edit prompt isn’t shown

  • 6. [preexisting issue] Hide the whole field when it’s empty and user cannot edit (see here) (in Viewer, fixed by Unknown text for user without Edit Metadata permission #3303)

Ideally, we would add reconciliation UI for multiple “tokens” here exactly as we have for labels, just keywords-like grey, (common ones could be removed from all selected images, those present only on some images, could be added to all etc.). We would use the same UI for (editing) keywords and other fields holding multiple values too (eg., per spec, XMP dc:creator can contain an array, even though no suppliers are using it, really).

image

image

But even without this extra UI work, I hope pts. 2–6 can be fixed, so that they at least work correctly.

Tested?

  • locally and exhibits problems like described above
  • on TEST

@AWare
Copy link
Contributor

AWare commented Jan 29, 2021

Was looking at 1., and it's possible to save in the correct form- but the array is prone to reordering after save. And fixing it is nontrivial.

Kapture 2021-01-29 at 17 08 55

@paperboyo
Copy link
Contributor Author

paperboyo commented Jan 29, 2021

it's possible to save in the correct form

Cool! Not sure why I said it didn’t work, will check, was some time ago (but I defo haven’t invented this error, I’m not that clever)

the array is prone to reordering after save. And fixing it is nontrivial.

All other points much more important than fixing reordering. We can defo live with the reordering. Ideally alphabetical, but random’s cool too if hard.

@AWare
Copy link
Contributor

AWare commented Feb 1, 2021

https://github.com/guardian/grid/compare/aw-mk-people-power
I fixed the saving here, and rolled back my ordering fix (which didn't work)

@paperboyo
Copy link
Contributor Author

paperboyo commented Feb 1, 2021

I fixed the saving here

Yep! Confirmed it works, yay! (I think it may be the same for similarly conditional Copyright field: when you batch apply on the upload page, it, correctly, batch applies it also to the pics that had no people field before, so the effect of the change is not visible for them, because the UI won’t show this newly acquired field on those pics. Easiest would be not to make it conditional: a bit more clutter, but a signal that we care about people. That we are the people’s people ;-).

@akash1810
Copy link
Member

Both peopleInImage and labels are the same type (an array of strings). They both need to be editable on upload, preview and search pages. They both want to be batchable. They both want to be set aware (as in for multiple images, does it exist in the intersection or not).

I think there's an opportunity to re-work the metadata forms here:

  1. Create a generic "array of strings" form field component
  2. Adopt this new component for peopleInImage
  3. Migrate labels to this new component
  4. (bigger task) make forms generic and configurable to avoid the copy pasting of massive chunks of html

@paperboyo and I have spiked 1 and hope to have a PR soon.

@akash1810
Copy link
Member

Create a generic "array of strings" form field component

Alas, this generic component will only benefit fields of type Iterable[String] within ImageMetadata.

Labels sit under a different part of the image, namely within userMetadata which is of type Edits - we appear to convert Edits into argo objects for the client and so the generic solution isn't easily applied to this field. Will see if the labeller component could be repurposed somehow.

@paperboyo
Copy link
Contributor Author

After this pure goodness goes in, Show additional metadata will be another place where arrays need to be handled. And there, we cannot assume the nature of the field upfront!

image

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.

3 participants