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

Generic UI list editor #3531

Merged
merged 15 commits into from
Feb 23, 2022
Merged

Generic UI list editor #3531

merged 15 commits into from
Feb 23, 2022

Conversation

adrielulanovsky
Copy link
Contributor

@adrielulanovsky adrielulanovsky commented Nov 9, 2021

What does this change?

This creates a generic component for displaying and editing lists of strings, such as labels, people in image, keywords, etc.

How can success be measured?

Previous behaviour in fields that contained arrays of strings is maintained. When trying to display a new field which is an array of strings, the implementation should be straightforward using this component.

TBD: Do we want to maintain the behaviour and look of the people in image tokens? The generic component allows for the possibility of adding and deleting single tokens from all selected images. If we want to do that for people, then we should change the style to something similar to what Labels has, so we can add a + and an x to add / remove. I left the same style as the labels, but this can be changed accordingly.

Screenshots

Who should look at this?

@guardian/digital-cms

Tested? Documented?

  • locally by committer
  • locally by Guardian reviewer
  • on the Guardian's TEST environment
  • relevant documentation added or amended (if needed)

@adrielulanovsky adrielulanovsky force-pushed the Generic-UI-list-editor branch 6 times, most recently from 1a391f5 to e3e4c13 Compare November 26, 2021 21:02
@adrielulanovsky adrielulanovsky marked this pull request as ready for review November 26, 2021 21:03
@adrielulanovsky adrielulanovsky requested a review from a team as a code owner November 26, 2021 21:03
@paperboyo
Copy link
Contributor

paperboyo commented Nov 29, 2021

This is great work and will unlock so much, thank you Adriel. Just regarding functionality and looks:

  1. Unknown (click ✎ to add) appears for empty fields. Maybe here
    image
    it should just look like it does for labels?
    image
  2. I think it’s worth keeping labels (and Collections) looking distinct from all other types. Collections have configurable colours and labels should be quick-to-glance and recognise. I think all other arrays should have just one look, similar to current keywords. What do people think about this mockup? (no italics, as these probably were an afterthought of Unknown values):
    image
    (I have reused existing colours: #222, #999 and black and white for partial values to be consistent with labels)
  3. Kudos for keeping tooltips correct on Remove and Add to all!

@paperboyo paperboyo added the bbc label Nov 29, 2021
@paperboyo
Copy link
Contributor

Thanks for fixes, Ebu! Some minor design issues:

sdfsdf

  1. One pixel horizontal line at the bottom of the pill (kill it)
  2. Text turns italic when same value on multiple images (should stay upright)
  3. Hover states have wrong colour or no colour change at all (just choose something vaguely sensible looking at current labels?)

@paperboyo
Copy link
Contributor

paperboyo commented Dec 3, 2021

Much more importantly:

  1. Adding a new value erases all other values (it should only add a new one!):
    sdfsdf2
  2. Removing from all (clicking an X) doesn’t work for me: Remove person from all does nothing.
  3. Progress bar text is broken:
    image
  4. I think there may be a performance issue: batch editing should be as performant as with labels, so around <500 images should work. Trying To Add to all on 70 images killed my browser (latest Firefox)

@paperboyo
Copy link
Contributor

paperboyo commented Dec 14, 2021

1, 2, 3 fixed, yay!

Ad. 4 I don’t think it works correctly now. When I click Add people, an editable fields appears that contains values from all selected images and allows adding a new value that will apply to all of them. But:

  • this editing field has no indication that some of its values may be present only on some images (eg. Lightroom adds asterisk to them)
  • it contains empty strings surrounded by commas for images that have no value
  • when I add a new value, it overwrites values on all selected images with the whole contents of this editable field!

While such an editable field (after fixing all above problems) would indeed be a more powerful editing device, allowing for adding to all something that exists only on some images, editing the name of something being added only to some images, removing something from any etc, I think this is not a good UI device for the Browser (especially because now it’s for People only) for the following reasons:

  • it is different to how Labels currently work
  • it is much harder to understand (one needs to know and understand the asterisk and counter it into editing carefully)
  • it is easier to make mistakes in batch editing
  • it is much more work than what I propose below

I think we should (at least for now), just make Add people plus button behave like in Labels: present a user with an empty field for adding a new value(s) to all selected images. That’s it. The other pills (and values) would stay on selected images as they were before the edit, just the new value(s) would get added. Buttons on pills already allow removing from any/all and adding to all for any existing pills. So we will miss only editing from the above approach. For which, the workaround would be to add a new correct value and remove the old incorrect one. This is also way easier, I guess. If anyone disagrees, please shout!

Ad 5, 6 They are still broken for me in latest Firefox and Chrome: removing a person from multiple pictures causes progress bar to go haywire and doesn’t work.

As always, please let me know if anything isn’t clear or if you are seeing different behaviour. Or, obviously, if you have different opinion on how stuff should work/look!

@abdelrahmansd
Copy link
Contributor

abdelrahmansd commented Dec 14, 2021

pts. 5–6

the progress bar works in normal add and doesn't work on partial add, I'm looking at it now
and remove all people works for me but I will deploy it to our environment and test it again

@abdelrahmansd
Copy link
Contributor

Hi @paperboyo
could you confirm that when clicking on a label on the image it makes a search filter by this label?

Screenshot from 2022-01-24 13-11-54

@paperboyo
Copy link
Contributor

paperboyo commented Feb 1, 2022

Hi. Thanks for the latest fixes, coming along nicely!

Ad 4 This is still the case. Let me know if you (or anyone) disagrees about the proposed simplest route in this comment above.
Gif that tries to show problems described in the above comment:
multi-edit

  1. Progress bar seems detached from the underlying operation (the counter doesn’t decrease and progress bar disappears much earlier than the operation finishes):
    batch

  2. Only labels should be blue. All the other pills (People, Keywords) should be grey. So let’s have Keywords in the People colour:
    image

@prout-bot
Copy link

Seen on cropper, kahuna, media-api, metadata-editor (created by @adrielulanovsky and merged by @andrew-nowak 10 minutes and 58 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on usage, collections, image-loader, image-loader-projection, thrall, leases (created by @adrielulanovsky and merged by @andrew-nowak 11 minutes and 3 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on auth (created by @adrielulanovsky and merged by @andrew-nowak 11 minutes and 11 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants