harmen/hypha
1
0
Fork 0
Code Issues 142 Pull requests 4 Projects Releases Packages Wiki Activity

Featured image #342

Merged
laurensmartina merged 12 commits from featured-image into master 2020-10-06 14:00:54 +00:00
Conversation 25 Commits 12 Files changed 10 +372 -182
laurensmartina commented 2020-09-25 06:05:58 +00:00 (Migrated from github.com)
Copy link

adds featured image at peer_reviewed_article.

adds featured image at peer_reviewed_article.
matthijskooijman (Migrated from github.com) reviewed 2020-09-28 12:19:04 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

Looks good, very comfortable to review with all these nice and small commits :-)

I left some small remarks inline.

Furthermore:

  • I think that the festival module could also be updated to use the auto-generated shadowfield rather than defining its own shadow field (which is a bit weird because it mixes field names in the handleImageUpload call currently).
  • If the festival module is updated, I think that handleImageUpload could be generalized to a handleFileUploads($_FILES) that just processes all fields in $form->file_fields, rather than just one, so callers need less checking and less hardcoding of the form structure in the processing code.
Looks good, very comfortable to review with all these nice and small commits :-) I left some small remarks inline. Furthermore: - I think that the festival module could also be updated to use the auto-generated shadowfield rather than defining its own shadow field (which is a bit weird because it mixes field names in the `handleImageUpload` call currently). - If the festival module is updated, I think that `handleImageUpload` could be generalized to a `handleFileUploads($_FILES)` that just processes *all* fields in `$form->file_fields`, rather than just one, so callers need less checking and less hardcoding of the form structure in the processing code.
system/assets/hypha-preview-image-file-field.js
matthijskooijman (Migrated from github.com) commented 2020-09-28 11:58:00 +00:00
Copy link

I think this might need to be scoped to the containing form tag, in case multiple forms with the same name are present (not possible now, but can happen later). I think something like this would work:

 $('input[name="' + inputName + '"][type="file"]', $preview.closest('form'));
I think this might need to be scoped to the containing `form` tag, in case multiple forms with the same name are present (not possible now, but can happen later). I think something like this would work: ```js $('input[name="' + inputName + '"][type="file"]', $preview.closest('form')); ```
matthijskooijman (Migrated from github.com) commented 2020-09-28 12:00:44 +00:00
Copy link

Why is this needed?

Why is this needed?
matthijskooijman (Migrated from github.com) commented 2020-09-28 12:02:17 +00:00
Copy link

Does this produce a meaningful URL, or a data url? If the latter, there's no real point in saving it into the shadow field in updateSrc, I think?

Does this produce a meaningful URL, or a data url? If the latter, there's no real point in saving it into the shadow field in `updateSrc`, I think?
matthijskooijman (Migrated from github.com) commented 2020-09-28 12:06:59 +00:00
Copy link

Why not merge toggle and updateSrc into one function?

I was also thinking hat if you use the value of the shadow field here, rather than the preview src, we could remove the preview handling on the PHP side, but that would prevent the preview from being resized (which might not be terrible, of course).

Why not merge `toggle` and `updateSrc` into one function? I was also thinking hat if you use the value of the shadow field here, rather than the preview src, we could remove the preview handling on the PHP side, but that would prevent the preview from being resized (which might not be terrible, of course).
system/core/HyphaImage.php
@ -0,0 +73,4 @@
* Take an uploaded image file, move it into the data
* directory and return the corresponding HyphaImage
* instance.
*
matthijskooijman (Migrated from github.com) commented 2020-09-28 11:38:11 +00:00
Copy link

You added translations for this, but are not using them here.

You added translations for this, but are not using them here.
matthijskooijman (Migrated from github.com) commented 2020-09-28 11:39:11 +00:00
Copy link

Did you intentionally remove this check?

Did you intentionally remove this check?
system/core/document.php
@ -482,6 +503,13 @@
}
matthijskooijman (Migrated from github.com) commented 2020-09-28 11:46:50 +00:00
Copy link

Maybe use array_key_exists rather than empty in these two lines, for consistency with the lines below?

Maybe use `array_key_exists` rather than `empty` in these two lines, for consistency with the lines below?
system/datatypes/peer_reviewed_article.php
@ -481,2 +495,3 @@
// create form
$form = $this->createEditForm($request->getPostData());
$form = $this->createEditForm($formData);
matthijskooijman (Migrated from github.com) commented 2020-09-28 12:15:13 +00:00
Copy link

Why add the ROOT_PATH here? That only hurts portability (moving the hypha installation)? Leaving it out also prevents having to remove it again when inserting data into the form values later? Also, when using this filename, you'd normally pass it to the HyphaFile constructor, which also needs it without the ROOT_PATH prepended.

Why add the `ROOT_PATH` here? That only hurts portability (moving the hypha installation)? Leaving it out also prevents having to remove it again when inserting data into the form values later? Also, when using this filename, you'd normally pass it to the `HyphaFile` constructor, which also needs it *without* the `ROOT_PATH` prepended.
system/datatypes/peer_reviewed_article.php
@ -1196,2 +1220,4 @@
</div>
</div>
<div class="section" style="padding:5px; margin-bottom:5px; position:relative;">
<div class="input-wrapper field_type_editor field_name_[[field-name-text]]">
matthijskooijman (Migrated from github.com) commented 2020-09-28 12:11:45 +00:00
Copy link

This would probably be more readable as an if statement.

This would probably be more readable as an `if` statement.
system/languages/en.php
@ -94,3 +94,3 @@
"error-loading-html" => "setNodeHTML: error when loading HTML",
"too-big-file" => "Error: file must be less than ",
"too-big-file" => "Error: file must be less than [[upload-max-filesize]]",
"invalid-image-file" => "Invalid image file, must be a valid image less than ",
matthijskooijman (Migrated from github.com) commented 2020-09-28 11:35:13 +00:00
Copy link

You changed this translation, but not the place it is called.

You changed this translation, but not the place it is called.
system/languages/en.php
matthijskooijman (Migrated from github.com) commented 2020-09-28 11:37:11 +00:00
Copy link

This uses _ while variables elsewhere (including too-big-file above) uses -. Better also use - here (no need to strictly match the php.ini variable here, I think).

This uses `_` while variables elsewhere (including `too-big-file` above) uses `-`. Better also use `-` here (no need to strictly match the php.ini variable here, I think).
laurensmartina (Migrated from github.com) reviewed 2020-09-28 16:28:38 +00:00
system/languages/en.php
@ -94,3 +94,3 @@
"error-loading-html" => "setNodeHTML: error when loading HTML",
"too-big-file" => "Error: file must be less than ",
"too-big-file" => "Error: file must be less than [[upload-max-filesize]]",
"invalid-image-file" => "Invalid image file, must be a valid image less than ",
laurensmartina (Migrated from github.com) commented 2020-09-28 16:28:38 +00:00
Copy link

good catch! fixed

good catch! fixed
laurensmartina (Migrated from github.com) reviewed 2020-09-28 16:56:45 +00:00
system/languages/en.php
laurensmartina (Migrated from github.com) commented 2020-09-28 16:56:44 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-09-28 17:03:08 +00:00
system/core/HyphaImage.php
@ -0,0 +73,4 @@
* Take an uploaded image file, move it into the data
* directory and return the corresponding HyphaImage
* instance.
*
laurensmartina (Migrated from github.com) commented 2020-09-28 17:03:08 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-09-28 17:21:59 +00:00
system/core/HyphaImage.php
@ -0,0 +73,4 @@
* Take an uploaded image file, move it into the data
* directory and return the corresponding HyphaImage
* instance.
*
laurensmartina (Migrated from github.com) commented 2020-09-28 17:21:59 +00:00
Copy link

Nope, placed it back

Nope, placed it back
laurensmartina (Migrated from github.com) reviewed 2020-09-28 17:28:13 +00:00
system/core/document.php
@ -482,6 +503,13 @@
}
laurensmartina (Migrated from github.com) commented 2020-09-28 17:28:13 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-09-28 18:40:59 +00:00
system/assets/hypha-preview-image-file-field.js
laurensmartina (Migrated from github.com) commented 2020-09-28 18:40:58 +00:00
Copy link

I ran into the problem that the label was still working on the hidden input. I just fixed this issue by disabling the input. :-)

I ran into the problem that the label was still working on the hidden input. I just fixed this issue by disabling the input. :-)
laurensmartina (Migrated from github.com) reviewed 2020-09-28 19:07:38 +00:00
system/assets/hypha-preview-image-file-field.js
laurensmartina (Migrated from github.com) commented 2020-09-28 19:07:38 +00:00
Copy link

True, i just need to put something there, so we can detect if the image remained the same, got replaced or removed.

True, i just need to put something there, so we can detect if the image remained the same, got replaced or removed.
laurensmartina (Migrated from github.com) reviewed 2020-09-28 19:50:42 +00:00
system/assets/hypha-preview-image-file-field.js
laurensmartina (Migrated from github.com) commented 2020-09-28 19:50:41 +00:00
Copy link

I was also thinking hat if you use the value of the shadow field here, rather than the preview src, we could remove the preview handling on the PHP side

This would not work because the shadow field value does not contain the correct path.

> I was also thinking hat if you use the value of the shadow field here, rather than the preview src, we could remove the preview handling on the PHP side This would not work because the shadow field value does not contain the correct path.
matthijskooijman (Migrated from github.com) reviewed 2020-09-28 20:30:08 +00:00
system/assets/hypha-preview-image-file-field.js
matthijskooijman (Migrated from github.com) commented 2020-09-28 20:30:08 +00:00
Copy link

What does "working on" mean for a label anyway? The label is still shown, and associated with the same input, which is hidden. Doesn't sound (semantically) incorrect? Does this confuse the browser somehow? You could argue that the label should now associate with the preview or the remove button, but I'm not so sure this is possible or useful?

What does "working on" mean for a label anyway? The label is still shown, and associated with the same input, which is hidden. Doesn't sound (semantically) incorrect? Does this confuse the browser somehow? You could argue that the label should now associate with the preview or the remove button, but I'm not so sure this is possible or useful?
matthijskooijman (Migrated from github.com) reviewed 2020-09-28 20:32:16 +00:00
system/assets/hypha-preview-image-file-field.js
matthijskooijman (Migrated from github.com) commented 2020-09-28 20:32:15 +00:00
Copy link

But you do not need to detect "replaced" using the hidden field, I think, since that is detected when a new file upload has happened in the file input. Removed does need to be detected, but that can just empty out the field?

If we add a library-browser type thing later (where you can select existing images), you can just put the already known public URL of these images in the input field as well.

But you do not need to detect "replaced" using the hidden field, I think, since that is detected when a new file upload has happened in the file input. Removed does need to be detected, but that can just empty out the field? If we add a library-browser type thing later (where you can select existing images), you can just put the already known public URL of these images in the input field as well.
matthijskooijman (Migrated from github.com) reviewed 2020-09-28 20:32:55 +00:00
system/assets/hypha-preview-image-file-field.js
matthijskooijman (Migrated from github.com) commented 2020-09-28 20:32:55 +00:00
Copy link

Yeah, I guess. Still, merging toggle and updateSrc seems reasonable?

Yeah, I guess. Still, merging `toggle` and `updateSrc` seems reasonable?
laurensmartina (Migrated from github.com) reviewed 2020-09-28 21:54:13 +00:00
system/assets/hypha-preview-image-file-field.js
laurensmartina (Migrated from github.com) commented 2020-09-28 21:54:13 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-09-29 00:00:27 +00:00
system/assets/hypha-preview-image-file-field.js
laurensmartina (Migrated from github.com) commented 2020-09-29 00:00:27 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-09-29 00:00:37 +00:00
system/assets/hypha-preview-image-file-field.js
laurensmartina (Migrated from github.com) commented 2020-09-29 00:00:37 +00:00
Copy link

merged

merged
laurensmartina (Migrated from github.com) reviewed 2020-09-29 00:03:02 +00:00
system/datatypes/peer_reviewed_article.php
@ -481,2 +495,3 @@
// create form
$form = $this->createEditForm($request->getPostData());
$form = $this->createEditForm($formData);
laurensmartina (Migrated from github.com) commented 2020-09-29 00:03:02 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-09-29 05:49:07 +00:00
system/datatypes/peer_reviewed_article.php
@ -1196,2 +1220,4 @@
</div>
</div>
<div class="section" style="padding:5px; margin-bottom:5px; position:relative;">
<div class="input-wrapper field_type_editor field_name_[[field-name-text]]">
laurensmartina (Migrated from github.com) commented 2020-09-29 05:49:07 +00:00
Copy link

If the ROOT_PATH is not saved, this whole part can be removed.

If the `ROOT_PATH` is not saved, this whole part can be removed.
laurensmartina (Migrated from github.com) reviewed 2020-09-29 06:02:29 +00:00
system/assets/hypha-preview-image-file-field.js
laurensmartina (Migrated from github.com) commented 2020-09-29 06:02:28 +00:00
Copy link

It confusing for the user if a file upload dialog is triggered while that input field is hidden. I've added a comment. :-)

It confusing for the user if a file upload dialog is triggered while that input field is hidden. I've added a comment. :-)
laurensmartina commented 2020-09-30 07:25:27 +00:00 (Migrated from github.com)
Copy link

Processed all comments. @matthijskooijman please review the changes.

I think that the festival module could also be updated to use the auto-generated shadowfield rather than defining its own shadow field (which is a bit weird because it mixes field names in the handleImageUpload call currently).

agreed

If the festival module is updated, I think that handleImageUpload could be generalized to a handleFileUploads($_FILES) that just processes all fields in $form->file_fields, rather than just one, so callers need less checking and less hardcoding of the form structure in the processing code.

agreed

Let's fix the festival module after merging this PR.

Processed all comments. @matthijskooijman please review the changes. > I think that the festival module could also be updated to use the auto-generated shadowfield rather than defining its own shadow field (which is a bit weird because it mixes field names in the handleImageUpload call currently). agreed > If the festival module is updated, I think that handleImageUpload could be generalized to a handleFileUploads($_FILES) that just processes all fields in $form->file_fields, rather than just one, so callers need less checking and less hardcoding of the form structure in the processing code. agreed Let's fix the festival module after merging this PR.
matthijskooijman (Migrated from github.com) reviewed 2020-10-05 08:45:21 +00:00
system/assets/hypha-preview-image-file-field.js
matthijskooijman (Migrated from github.com) commented 2020-10-05 08:45:21 +00:00
Copy link

Ah, I didn't realize that clicking the label triggers the input field, good point.

Ah, I didn't realize that clicking the label triggers the input field, good point.
matthijskooijman (Migrated from github.com) approved these changes 2020-10-05 09:43:32 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

I left one more question, and wondered: Should the featured image be shown along with the excerpt (for logged in users)?

I left one more question, and wondered: Should the featured image be shown along with the excerpt (for logged in users)?
system/core/document.php
matthijskooijman (Migrated from github.com) commented 2020-10-05 09:23:45 +00:00
Copy link

Why does this attribute exist?

Why does this attribute exist?
laurensmartina (Migrated from github.com) reviewed 2020-10-05 12:38:12 +00:00
system/core/document.php
laurensmartina (Migrated from github.com) commented 2020-10-05 12:38:12 +00:00
Copy link

true, not needed. fixed

true, not needed. fixed
laurensmartina commented 2020-10-05 12:40:58 +00:00 (Migrated from github.com)
Copy link

I left one more question, and wondered: Should the featured image be shown along with the excerpt (for logged in users)?

Good idea! I'll implement it.

> I left one more question, and wondered: Should the featured image be shown along with the excerpt (for logged in users)? Good idea! I'll implement it.
matthijskooijman commented 2020-10-05 17:22:58 +00:00 (Migrated from github.com)
Copy link

@laurensmartina, I added one small simplification, see the added fixup commit. If that looks good to you, I'll squash and merge this tomorrow.

@laurensmartina, I added one small simplification, see the added fixup commit. If that looks good to you, I'll squash and merge this tomorrow.
matthijskooijman commented 2020-10-06 14:00:49 +00:00 (Migrated from github.com)
Copy link

Rebased on top of master, merging next.

Rebased on top of master, merging next.
Sign in to join this conversation.
Reviewers
No reviewers
matthijskooijman
Labels
Clear labels   
Component: User interface

   
Component: Wymeditor

   
Help wanted

   
Level: Difficult

Structural changes, need discussion

  
Level: Easy

Under 4 hours of work

  
Level: Moderate

Under 16 hours of work

  
Pagetype: Festival

   
Pagetype: Mailinglist

   
Pagetype: Peer reviewed article

   
Pagetype: Text

   
Privacy GDPR AVG

   
status: has conflicts

   
Status: Needs changes

   
Status: Needs discussion

   
Status: Needs review

   
Status: Ready to merge

   
Status: Waiting for response

   
Type: Bug

   
Type: Enhancement

   
Type: Question

   
Usecase: De Stadsbron

   
Usecase: Koppelting

   
Usecase: MeetjeStad

   
Value: Coders

Enhancement of codebase, API, documentation

  
Value: Security

   
Value: Users

Improve user experience, add new features

  
Value: Visitors

Improve navigation, styling

No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
harmen/hypha!342
No description provided.