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

EZP-29104: Implemented UI for ImageAsset Field Type #580

Merged
merged 1 commit into from
Sep 12, 2018
Merged

EZP-29104: Implemented UI for ImageAsset Field Type #580

merged 1 commit into from
Sep 12, 2018

Conversation

adamwojs
Copy link
Member

@adamwojs adamwojs commented Jul 26, 2018

Question Answer
Tickets https://jira.ez.no/browse/EZP-29104
Depends on ezsystems/repository-forms#246, ezsystems/ezpublish-kernel#2403
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? no
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Extract translations
  • Ready for Code Review

@ezrobot

This comment has been minimized.

@adamwojs adamwojs changed the title [WIP] EZP-29104: Impl. ImageAsset field type EZP-29104: Impl. ImageAsset field type Jul 27, 2018
@ezrobot

This comment has been minimized.

@@ -37,3 +37,6 @@ system:
multiple: false
content_on_the_fly:
allowed_content_types: ['image']
image_asset:
multiple: false
visible_tabs: ['browse', 'search', 'bookmarks']
Copy link
Member

Choose a reason for hiding this comment

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

Why you should not be able to create content while picking from UDW?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I going to check this with PM team 😉

const SELECTOR_INPUT_FILE = 'input[type="file"]';
const SELECTOR_LABEL_WRAPPER = '.ez-field-edit__label-wrapper';
const SELECTOR_INPUT_DESTINATION_CONTENT_ID = '.ez-field-edit__destination_content_id';

Copy link
Member

Choose a reason for hiding this comment

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

No need to separate const

Copy link
Member

Choose a reason for hiding this comment

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

Empty lines are not needed in const

const imageAssetMapping = eZ.adminUiConfig.imageAssetMapping;

class EzImageAssetPreviewField extends eZ.BasePreviewField {

Copy link
Member

Choose a reason for hiding this comment

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

Please remove empty line

* @param {String} languageCode
*/
createImage(file, languageCode) {
const assetCreateUri = '/asset/image';
Copy link
Member

Choose a reason for hiding this comment

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

Should be generate by the JS routing

};

this.toggleLoading(true);
fetch(assetCreateUri, options).then((response) => {
Copy link
Member

Choose a reason for hiding this comment

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

Please move this function to some method

canSelectContent,
title,
multiple: false,
startingLocationId: parseInt(imageAssetMapping['parent_location_id'], 10),
Copy link
Member

Choose a reason for hiding this comment

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

This can't be moved to the config?

super.init();

this.btnSelect = this.fieldContainer.querySelector('.ez-data-source__btn-select');
this.btnSelect.addEventListener('click', (event) => {
Copy link
Member

Choose a reason for hiding this comment

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

I do not see a point of this middle function which will only preventDefault, you can do it in openUDW

}

.ez-field-edit__spinner {

Copy link
Member

Choose a reason for hiding this comment

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

this can be remove

@@ -0,0 +1,78 @@
.ez-field-edit--ezimageasset {
.ez-field-edit-preview {
padding: 1.25rem 0 1.25rem 1.25rem;
Copy link
Member

Choose a reason for hiding this comment

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

Indentation 4 spaces

}
}

.ez-field-edit {
Copy link
Member

Choose a reason for hiding this comment

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

This will override styles for all fields. Cannot be done like this.

@adamwojs
Copy link
Member Author

PR rebased and updated according to @dew326 suggestions.

const SELECTOR_INPUT_FILE = 'input[type="file"]';
const SELECTOR_LABEL_WRAPPER = '.ez-field-edit__label-wrapper';
const SELECTOR_INPUT_DESTINATION_CONTENT_ID = '.ez-field-edit__destination_content_id';

Copy link
Member

Choose a reason for hiding this comment

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

Empty lines are not needed in const

const SELECTOR_FIELD = '.ez-field-edit--ezimageasset';
const SELECTOR_INPUT_FILE = 'input[type="file"]';
const SELECTOR_LABEL_WRAPPER = '.ez-field-edit__label-wrapper';
const SELECTOR_INPUT_DESTINATION_CONTENT_ID = '.ez-field-edit__destination_content_id';
Copy link
Member

Choose a reason for hiding this comment

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

not correct BEM class: ez-field-edit__destination-content-id

method: 'POST',
headers: {
'Accept': 'application/json',
//'X-CSRF-Token': token
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 the CSRF token should be sent in POST requests .

},
body: form,
mode: 'same-origin',
credentials: 'same-origin'
Copy link
Member

Choose a reason for hiding this comment

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

Do you have Prettier installed? I think we have rule for trailingComma

};

this.toggleLoading(true);
fetch(assetCreateUri, options)
Copy link
Member

Choose a reason for hiding this comment

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

I would add empty line before this fetch

}
}

.ez-field-edit--ezimageasset {
Copy link
Member

Choose a reason for hiding this comment

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

This can be combined with the rules above

}

.ez-field-edit--ezimageasset {
&__preview-wrapper {
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 this will be compiled to: ez-field-edit--ezimageasset__preview-wrapper and this is not correct usage of BEM

.ez-field-preview--ezimageasset {
display: flex;

.ez-imageasset__wrapper {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like not correct BEM

{% endblock %}

{% block ezplatform_fieldtype_ezimageasset_widget %}
<div class="ez-data-source__message--main">
Copy link
Member

Choose a reason for hiding this comment

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

Seems like not correct BEM usage

</div>

{% if max_file_size is defined and max_file_size > 0 %}
<div class="ez-data-source__message--filesize">
Copy link
Member

Choose a reason for hiding this comment

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

The same, not correct BEM


$form = $this->formFactory->assetUpload($data, 'form');
$form->handleRequest($request);
if ($form->isValid()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's advised by Symfony to use $form->isSubmitted() && $form->isValid() together.

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

class AssetController extends Controller
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt it's a good idea to keep the Controller in AdminUI - what if we need this fieldtype to be available for frontend editing?

method: 'POST',
headers: {
'Accept': 'application/json',
//'X-CSRF-Token': token
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented out code should not be commited.

@@ -223,3 +223,6 @@
{{- form_row(form.textRows) -}}
</div>
{% endblock %}

{% block ezimageasset_field_definition_edit %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is an empty block needed?


{% set widget_container_block = block('ezplatform_fieldtype_ezimageasset_widget_container') %}

{{ block('form_row_fieldtype') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

You are duplicating a lot of variables from block binary_base_row (defined in binary_base.html.twig). Can't you override only some of the variables (it might require to do some changes to binary_base_row to make it possible) and display binary_base_row?

</tr>
<tr>
<td>{{ 'ezimageasset.size'|trans|desc('Size') }}:</td>
<td>{{ field.value.fileSize|ez_file_size( 1 ) }}</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Spaces no needed: ez_file_size(1)

{
$data = new AssetUploadData();

$form = $this->formFactory->assetUpload($data, 'form');
Copy link
Contributor

Choose a reason for hiding this comment

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

Name form feels too generic to me.

const assetCreateUri = global.Routing.generate('ezplatform.asset.upload');
const form = new FormData();

form.append('form[languageCode]', languageCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't manually compose Data object here. Please use existing form:

        const request = new Request(form.action, {
            method: 'POST',
            body: new FormData(form),
            mode: 'same-origin',
            credentials: 'same-origin',
        });

@dew326
Copy link
Member

dew326 commented Aug 1, 2018

I have been thinking about the whole idea of using a form in this case and this makes no sense.
As far as I can see you are creating a form, rendering only a few fields and send two fields in POST request. The endpoint gets the form and returning JSON response (so there is no possibility to submit this form in a regular way). Thus we do not need this form at all.

This should be done like this:

  • no form
  • a regular POST endpoint which takes in the body:
{
    languageCode: languageCode,
    file: file
}
  • JS sending POST with the body and CSRF token

@webhdx
Copy link
Contributor

webhdx commented Aug 1, 2018

Totally overlooked how it's really done yesterday, sorry. I second what @dew326 said. Using Form is convenient, especially when validation and creating data object is needed but in your case it's totally misused. It works but it will be confusing for anyone who maintains the code in the future.

}

return new JsonResponse([
'status' => 'error',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this status field really needed?

$this->languageCode = $languageCode;
}

public function getFile(): ?UploadedFile
Copy link
Contributor

Choose a reason for hiding this comment

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

missing docblock here and below

@adamwojs
Copy link
Member Author

adamwojs commented Aug 2, 2018

Ready for the second round of code review @dew326, @webhdx, @mikadamczyk.

(function(global, doc, eZ, React, ReactDOM, Translator) {
const SELECTOR_FIELD = '.ez-field-edit--ezimageasset';
const SELECTOR_INPUT_FILE = 'input[type="file"]';
const SELECTOR_INPUT_DESTINATION_CONTENT_ID = '.ez-data-source--ezimageasset__destination-content-id';
Copy link
Member

Choose a reason for hiding this comment

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

This in not correct. You shouldn't add children to class with modifier.
ez-data-source__destination-content-id

{% set imageAlias = ez_image_alias( field, versionInfo, parameters.alias|default( 'original' ) ) %}
{% set src = imageAlias ? asset( imageAlias.uri ) : "//:0" %}

<div class="ez-imageasset__wrapper">
Copy link
Member

Choose a reason for hiding this comment

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

ez-field-preview__image-wrapper

@adamwojs
Copy link
Member Author

adamwojs commented Aug 3, 2018

PR ready for the final code review @webhdx @dew326 @mikadamczyk

dew326
dew326 previously requested changes Aug 3, 2018
</div>
{% endif %}

{{- form_widget(form.file, {'attr': attr}) -}}
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 we drop the idea of creating a form for this?

default:
controller: 'EzPlatformAdminUiBundle:ContentView:locationView'
template: '@ezdesign\fieldtypes\preview\ezimageasset.html.twig'
match: true
Copy link
Contributor

Choose a reason for hiding this comment

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

missing empty line

@@ -20,7 +22,10 @@
{% set widget_wrapper_attr = widget_wrapper_attr|merge({'hidden': 'hidden'}) %}
{% endif %}

{% set widget_container_block = block('binary_base_widget_container') %}
{% if not widget_container_block_name is defined %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not {% if widget_container_block_name is not defined %}? Looks better and reads better.

@webhdx

This comment has been minimized.

@barbaragr
Copy link

barbaragr commented Aug 23, 2018

I Error 500 after deleting Image used in ImageAssets field

  1. Create Content Type with ImageAsset FieldType
  2. Go to Content/Content structure and create new Content Item based on Content Type from step 1
  3. Upload an image and publish
  4. Go to Media/Images and send to Trash image from previous step
  5. Go back to Content/Content structure and go to view mode of recently published CI
    Actual result: error 500: Type error: Argument 1 passed to eZ\Publish\Core\SignalSlot\BookmarkService::isBookmarked() must be an instance of eZ\Publish\API\Repository\Values\Content\Location, null given, called in /var/www/ezplatformee/vendor/ezsystems/ezplatform-admin-ui/src/bundle/Controller/ContentViewController.php on line 366

II Error 500 after editing added image in Media/Image

  1. Create Content Type with ImageAsset FieldType
  2. Go to Content/Content structure and create new Content Item based on Content Type from step 1
  3. Upload an image and publish
  4. Go to Media/Images and find the image and click Edit button to go to edit mode
  5. Remove the image from field Image (Trash button) and publish
  6. Go back to Content/Content structure and go to view mode of recently published CI
    Actual result: Error 500: An exception has been thrown during the rendering of a template ("Argument 'BinaryFile::id' is invalid: 'NULL' is wrong value in class 'BinaryFile'").

III Question:
When I'm creating a CI with ImageAssets and I have uploaded an image, but next I remove it using TRash button, should it stay in images repository? Specifications says only about uploading the image and saving a draft. In my situation I saved nothing, so maybe it shouldn't be publish in Media/Images yet?

IV Question:
For scenario:
When I create a new Article
And decide to select an existing image for the Photo field
And try to select any content item which is not an Image content type
Then I can not select the content item
Now I can select, but I can't confirm. Maybe It could be disabled like some CIs are disabled in UDW when user want to swap locations (containers vs non containers). But I'm just asking :)

V User receive error when tries to upload a file which is not an image

  1. Create Content Type with ImageAsset FieldType
  2. Go to Content/Content structure and create new Content Item based on Content Type from step 1
  3. Try to upload, using drag and drop, for example txt file
    Actual result: Error while creating image asset is displayed. For ezimage when user drags and drops forbidden file, nothing happens.

Copy link

@barbaragr barbaragr left a comment

Choose a reason for hiding this comment

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

VI Permissions:
Preconditions: created CT with ImageAssets field type

  1. Create user Editor
  2. Change Editor's assignments: left only Subtree of Location: /Home
  3. Log in as an Editor
  4. Go to Content/Content structure and click Create
  5. Choose CT with Image Assets field type
  6. Drag and drop an image
    Actual result: We receive a message Error while creating image asset - we could be more specific and display sth about permissions, because error occurs if user does't have permissions to create a CI anywhere except Home.

@SylvainGuittard
Copy link
Contributor

Thanks @barbaragr for testing.

I Error 500 after deleting Image used in ImageAssets field
I updated the specifications. Please check it again.

II Error 500 after editing added image in Media/Image
This field is not mandatory in the Image content type. So the best solution is to not display the image but the name of the content item. We keep the Open in a new tab link for the user to edit it again.

III Question:
The image should be deleted

IV Question:

Maybe It could be disabled like some CIs are disabled in UDW when user want to swap locations (containers vs non containers).

Totally agree.

@adamwojs
Copy link
Member Author

adamwojs commented Sep 5, 2018

@barbaragr , @SylvainGuittard

  1. This problem is already solved by EZP-29342: Deleting related object for ezobjectrelations field throws error 500 #581. Please confirm it by cherry picking 501190d8ec6d88bffb37ed6455ebee89e78a3975.

  2. I already handled this case by displaying Image asset is not available message but definitely it might be improved. I see what it can do 😉

zrzut ekranu z 2018-09-05 10-08-04

  1. This is edge case but worth to mention. As the image content item is created and published immediately after upload, this gives a time frame to reuse it in other content. if "Trash" button in content editor will delete related content item so it need same confirmation modal window as impl. as part of EZP-29408: As an editor I want to delete a content item with an image asset field #591 and it need to be permission aware.

PS. I will update PR today 😉

@ViniTou
Copy link
Contributor

ViniTou commented Sep 5, 2018

I can confirm that 1 is solved just by rebasing.

@adamwojs
Copy link
Member Author

adamwojs commented Sep 6, 2018

PR updated according to @barbaragr suggestions and ready to second round of QA.

  1. I decided to handle this issue as @SylvainGuittard suggested. Now when image is empty, the content name will be displayed:

image_value_is_empty

When the related content item is deleted or current user doesn't have permission to see it the the following message will be display:

image_asset_is_not_available

From technical POV, I implemented Parameter Provider (similar as in case of ezrelation and ezrelation list field types) in which pass available parameter to FT block. See https://github.com/ezsystems/ezpublish-kernel/pull/2403/files#diff-a69bf20f91185b4b1c044ed1b71d5bceR41

AD3) No changes here.

AD4) No changes here. I'm missing proper API in UDW to be able to do this.

AD5) Behavior has been aligned with the ezimage field type. You cannot drop file which are not images into ezimageasset FT, also proper filter is now active by default in the file select dialog.

When you try to force upload of non-image file the "The file is not valid image" error will be displayed:

file_is_not_a_valid_image

AD6) In this case the following error will be displayed:
insufficent_permissions

@barbaragr
Copy link

barbaragr commented Sep 6, 2018

AD I Image is still visible after deleting Image used in ImageAssets field

  1. Create Content Type with ImageAsset FieldType
  2. Go to Content/Content structure and create new Content Item based on Content Type from step 1
  3. Upload an image and publish
  4. Go to Media/Images and send to Trash image from previous step
  5. Go back to Content/Content structure and go to view mode of recently published CI
    Actual result: In view mode user sees a message: Image asset is not available (related content has been deleted or insufficient permissions) but if he go to the Edit mode, he can still see the image in image asset field.

@barbaragr
Copy link

barbaragr commented Sep 6, 2018

AD II Question:

  1. Create Content Type with ImageAsset FieldType
  2. Go to Content/Content structure and create new Content Item based on Content Type from step 1
  3. Upload an image and publish
  4. Go to Media/Images and find the image and click Edit button to go to edit mode
  5. Remove the image from field Image (Trash button) and publish
    Actual result: According to @SylvainGuittard and @adamwojs in view mode content name is displayed (with a link to open in a new tab), but when I go to edit mode ezimageasset field is empty. When I change nothing and click Publish content name with a link is still visible. Are we OK with that? Or should it be overridden and This field is empty message should be displayed?

@barbaragr
Copy link

AD VI Question:
User receives a message: Error while creating image asset: User does not have access to 'read' 'content' It could by misleading, because, as default, editor has got all content policies and assign limitations for Home subtree. Maybe the information message should be more general?

@barbaragr
Copy link

VII I can't create Landing Page

  1. Go to Content/Content structure
  2. Click Create and choose Landing Page
    Actual result: There is no layout modal and user is unable to publish LP. Console shows: Uncaught TypeError: window.eZ.addConfig is not a function and Uncaught TypeError: Cannot read property 'config' of undefined

@SylvainGuittard
Copy link
Contributor

Actual result: According to @SylvainGuittard and @adamwojs in view mode content name is displayed (with a link to open in a new tab), but when I go to edit mode ezimageasset field is empty. When I change nothing and click Publish content name with a link is still visible. Are we OK with that? Or should it be overridden and This field is empty message should be displayed?

Displaying This field is empty message might confuse editors. Is it the field of the current content item or the content item image? I think we should include a link like in view mode.
image

@barbaragr
Copy link

barbaragr commented Sep 10, 2018

VIII Error when previewing LP after sending to trash image from image asset field

  1. Create Content Type with ImageAsset FieldType and Landing Page
  2. Go to Content/Content structure and create new Content Item based on Content Type from step 1
  3. Upload an image and publish
  4. Go to Media/Images and send to Trash image from previous step
  5. Go back to Content/Content structure and CI created in step 3
  6. Click Page/site to see page view and switch to edit mode
    Actual result: There is an error: An exception has been thrown during the rendering of a template ("Parameter "locationId" for route "_ezpublishLocation" must match "[^/]++" ("" given) to generate a corresponding URL.").

);

this.toggleLoading(false);
showErrorNotification({ message });
Copy link

@barbaragr barbaragr Sep 11, 2018

Choose a reason for hiding this comment

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

I think that these curly braces are needless. It should be:

showErrorNotification(message);

showErrorNotification takes Error object or string.

@barbaragr
Copy link

All right! To sum up:
AD I - OK
AD II - OK
AD III - works fine, updated spec (Discard of the parent content item using the image asset)
AD IV - follow up: https://jira.ez.no/browse/EZP-29589
AD V - OK
AD VI - Probably this will fix it ezsystems/ezpublish-kernel#2408 (I will check after merge)
AD VII - OK
AD VIII - OK

I didn't check DBP (blocked by: https://jira.ez.no/browse/EZEE-2308) and translations (blocked by: https://jira.ez.no/browse/EZEE-2316)

Copy link

@barbaragr barbaragr left a comment

Choose a reason for hiding this comment

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

Ready to merge @alongosz

Co-authored-by: Dawid Parafiński <dawid.parafinski@ez.no>
Co-authored-by: Jakub Brzegowski <symfiz@gmail.com>
@alongosz alongosz dismissed dew326’s stale review September 12, 2018 12:09

Seem already resolved

@alongosz alongosz changed the title EZP-29104: Impl. ImageAsset field type EZP-29104: Implemented UI for ImageAsset Field Type Sep 12, 2018
@alongosz alongosz merged commit 6db696b into ezsystems:master Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

10 participants