ILIAS icon indicating copy to clipboard operation
ILIAS copied to clipboard

[FEATURE] UI: Introduce dedicated image upload field

Open thibsy opened this issue 1 year ago • 9 comments

Hi folks,

I am happy to propose the public interface for a dedicated image upload field, which encourages users to provide alternate texts when necessary. The related FR can be found here: https://docu.ilias.de/goto_docu_wiki_wpage_5882_1357.html

As discussed in the UI-Clinic, we have decided to align this input with the existing file input, so we can reuse most of its functionality and reduce maintenance in the future. We also agreed, that the look-and-feel of the input needs some polishing. Therefore, I also updated the factory description of the file input, to match the new mockups more precisely.

I plan on using the EntityListing component to build the file-list internally. Maybe we could introduce some concrete FileUploadEntity in the future, to wrap up all of the components required to build this (progress bar, accordion, glyphs, ...). Let me know if this is a good idea.

Similar to the Rating input, I added an ImagePurpose enum to determine how the alternate text input should be handled. We currently know three different scenarios:

  • the consumer knows that uploaded images will be informative, so an alt-text input is required.
  • the consumer knows that uploaded images will be decorative, so an alt-text input is obsolete.
  • the consumer doesn't know what the image purpose is, so an alt-text may be required. For this case a radio group must be used, letting the user make this choice.

I am looking forward to your review!

Kind regards, @thibsy

thibsy avatar Apr 10 '24 15:04 thibsy

image-upload-mockup-desktop

thibsy avatar Apr 10 '24 15:04 thibsy

Hi @thibsy

Thx a lot for the clear description and differentiation of the three scenarios. We appreciate the concept of a Image Field Input to develop further parts of the concept.

Some Question:

  • [x] Small Screens: Will and if so, how will the display on small screens differ from the provided mocks.
  • [x] Image Field Input: Would it make sense, to understand the Image Field Upload as a special variation of the File Upload? You devised it as a Field on the same level on our Semantic Tree. Why?

Change or indicate why not:

  • [x] Error Display: It probably makes sense to display the error message above the field with file name, as is usually the case in forms.

Thx a lot, @Amstutz and @klees with kind support of @yvseiler

Amstutz avatar Apr 19 '24 06:04 Amstutz

Hi @Amstutz,

Thx for the feedback! Let me answer your questions/remarks:

  • Small screens: The file entries will essentially become more "squashed" but will keep the look and feel as much as possible. One notable layout change is the preview section of the new image input though, which will shift above the metadata inputs and will not remain next to them, at some specified breakpoint.
  • Image Field Input: The image field is definitely a special variation of the file input. I have changed the inheritance accordingly, which actually simplifies things :).
  • Error display: I agree, we could probably reuse the styling of form errors too. I will not adjust the mockups, but I'll keep this in mind.

Hope this helps!

Kind regards, @thibsy

thibsy avatar Apr 19 '24 09:04 thibsy

Jour Fixe, 29 APR 2024: We highly appreciate this PR and accept it for ILIAS 10 / trunk.

matthiaskunkel avatar Apr 29 '24 13:04 matthiaskunkel

Looking forward to the implementation, please re-assign us, as soon as we can review this.

Amstutz avatar Apr 30 '24 09:04 Amstutz

Hi @thibsy,,

the Technical Board looks into oldish PRs regularly, this is when this PR came up. Is there anything that we can do here to move this to some definite state, be it a merge or close?

Kind regards!

klees avatar Feb 04 '25 12:02 klees

Hi @thibsy

Please respond to @klees comment.

Kind regards

ckozianka avatar Feb 11 '25 08:02 ckozianka

Hi all,

This PR got stuck a while back because we tried to refactor the file input while/before implementing this new variation of a file input. The refactoring was meant to replace the underlying dependency used for client-side file upload handling. This also meant refactoring the JavaScript code to an ES6 module, which turned out to be a much more complicated task than anticipated, so we have decided to postpone the implementation of the image field and search for additional funding. That's what ultimately led us to this indefinite state.

Since @ckozianka and his organisation are funding the implementation of the new image field input (thank you very much!), we have decided to put the refactoring aside and ignore the sunken cost here, so we can ship the image field input for an early stage of ILIAS 11.

About the technicalities of this PR:

  • Metadata inputs: this feature heavily relies on the Field\HasDynamicInputs implementation of the Field\File input, which turns out to have some issues that affect this feature greatly. I have already fixed some of these issues (contained in this PR as well), but there are still some issues left which are not trivial to fix. See the list of issues here:
    • https://mantis.ilias.de/view.php?id=44148
    • https://mantis.ilias.de/view.php?id=44176
    • https://mantis.ilias.de/view.php?id=44192
    • https://mantis.ilias.de/view.php?id=44193
  • File upload library: since the refactoring could not be done, we are stuck with the old npm package dropzone. I will request this library in a separate PR and label it for discussion at the upcoming JF on Monday.

In summary: the Field\Image input of this PR is functional, if the input only allows to upload one image at a time (which is default). The issues mentioned above only affect inputs where multiple inputs are generated on the client. However, since this thing is functional, we could perhaps merge it before the issues are addressed, to make it available for development of ILIAS 11. Let me know what you think :).

The PR still needs some polish in form of improved example descriptions, translations, unit tests, etc.

We apologise to all involved parties for the delay here.

Kind regards, @thibsy

thibsy avatar Feb 14 '25 11:02 thibsy

Hi all,

Please answer the following questions:

  • [ ] Switchable Group: this is actually a bugfix, we should probably discuss it here: #9024. I will rebase here once the bugfix PRs are merged.
  • [x] +description "Effect": this behaviour actually doesn't exist. I phrased this paragraph differently.
  • [x] example 5: this is because none of the example implements data processing. I have added one where this will be visible.
  • [x] example 5 2: they were not yet translated, I have added some. The language group can gladly chip in here though.

Please consider the following suggestions. You do not need to follow those, but but please indicate shortly why you prefer to do otherwise:

  • [x] example 1: agree. done in #9029; I will rebase once the bugfix PRs have been merged.
  • [x] example 5 3: I fixed this issue, good catch :)!
  • [x] mime type: I reported and fixed the issue (https://mantis.ilias.de/view.php?id=44978). This behaviour most likely exists in all supported version.

Please implement the following changes:

  • [x] example 2: done.
  • [x] description "Composition": for the same reason as your example 1 remark, I would rather not describe the close Glyph. This could e.g. easily be a minus someday.
  • [x] example 1-4: I fixed the issue, you can have another look now.

Kind regards, @thibsy

thibsy avatar Apr 28 '25 10:04 thibsy

The issue mentioned above with missing language variables is a problem with the “Switchable Group” in general. The Mantis issue https://mantis.ilias.de/view.php?id=45569 was created for this purpose.

There is no further need for adjustments on the part of UI/UX or HTML validation.

I am therefore handing the issue over to you @amstutz & @klees for the remaining bug fixes.

yvseiler avatar Aug 19 '25 07:08 yvseiler