implement caption and meta-information for images #119 #344

2020-10-04T16:27:46Z

laurensmartina commented

2020-10-04 16:27:46 +00:00

(Migrated from github.com)

Fixes #119

matthijskooijman (Migrated from github.com) reviewed

2020-10-05 08:40:03 +00:00

matthijskooijman (Migrated from github.com) left a comment

I think I might be misunderstanding how all of this fits together, so I asked some questions inline. Maybe we should look at this together this afternoon.

system/core/pages.php

I would rename these functions to more descriptive names, maybe add_captions_to_all_images and add_caption_to_image?

I would rename these functions to more descriptive names, maybe `add_captions_to_all_images` and `add_caption_to_image`?

Doesn't this need a <figure> tag as well to group the caption with the image? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/figcaption

Doesn't this need a `<figure>` tag as well to group the caption with the image? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/figcaption

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

This seems weird: You enter an attribution, which is then saved as a "long description", and displayed as a caption, which seem three distinct things that I think we should not mix here? Maybe this is also something that @giplt could comment on?

IIUC this processes an "attribution" attribute returned from the image upload handler, but I see no changes in the upload handler?

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

					
				@ -54,0 +81,4 @@

							wym.registerModification();

						} else {

							wym.insertImage(imgAttrs);

						}

Why is this submitHandler completely new? I would think that adding attribution would only need to add one attribute to an existing handler. Is there maybe a hidden extra change here?

laurensmartina (Migrated from github.com) reviewed

2020-10-05 09:32:13 +00:00

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

True! A next feature will be that the upload will return "attribution". This code is unaware of the state of the upload and already prepared.

laurensmartina (Migrated from github.com) reviewed

2020-10-05 09:45:13 +00:00

system/core/pages.php

fixed!

laurensmartina (Migrated from github.com) reviewed

2020-10-05 09:49:31 +00:00

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

I've implemented it like it was requested by @tammoterhark, I think he had a clear vision about this.

laurensmartina (Migrated from github.com) reviewed

2020-10-05 09:59:13 +00:00

system/core/pages.php

Ah, I didn't know! fixed

laurensmartina (Migrated from github.com) reviewed

2020-10-05 10:06:36 +00:00

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

					
				@ -54,0 +81,4 @@

							wym.registerModification();

						} else {

							wym.insertImage(imgAttrs);

						}

No extra change, AFAIK the submitHandler needs to be overwritten in order to add a new attribute. Of course I'm open for suggestions.

matthijskooijman (Migrated from github.com) requested changes

2020-10-06 12:41:34 +00:00

matthijskooijman (Migrated from github.com) left a comment

I left a bunch of inline comments which I also fixed in two fixup commits that I'll push in a minute.

I did find some other problems that are still unfixed right now and might need us to rethink our approach. In particular:

The <figure> tag is a block-level tag, while <img> is inline, so adding the figure creates invalid HTML, causing at least my Firefox to drop the <figure>, it seems.
Any layout CSS applied to the image might break. In particular, the default theme supports e.g. the .left class to make the image floating and have text wrap around it, but this breaks when wrapped inside <figure>.
Inside the wymeditor, the caption is not shown, which makes it a bit harder to preview changes.

Not sure if there is an easy fix for these, so maybe we should be storing <figure> tags after all?

I left a bunch of inline comments which I also fixed in two fixup commits that I'll push in a minute. I did find some other problems that are still unfixed right now and might need us to rethink our approach. In particular: - The `<figure>` tag is a block-level tag, while `<img>` is inline, so adding the `figure` creates invalid HTML, causing at least my Firefox to drop the `<figure>`, it seems. - Any layout CSS applied to the image might break. In particular, the default theme supports e.g. the `.left` class to make the image floating and have text wrap around it, but this breaks when wrapped inside `<figure>`. - Inside the wymeditor, the caption is not shown, which makes it a bit harder to preview changes. Not sure if there is an easy fix for these, so maybe we should be storing `<figure>` tags after all?

system/core/pages.php

I think the xpath can be simplified to:

		foreach ($element->findXPath('//*[@id="main"]//img[@title or @data-attribution]') as $img) {

I think the xpath can be simplified to: ```suggestion foreach ($element->findXPath('//*[@id="main"]//img[@title or @data-attribution]') as $img) { ```

This adds an empty figcaption when no title exists, which looks weird. Also, I think that it would make sense to put the attribution inside the caption, so they can be styled together if needed, and then put the title in a span with a class so it can also be styled separately.

This adds an empty `figcaption` when no title exists, which looks weird. Also, I think that it would make sense to put the attribution inside the caption, so they can be styled together if needed, and then put the title in a span with a class so it can also be styled separately.

This can be shorted using the wrap method. Documentation was missing, so I submitted https://github.com/scotteh/php-dom-wrapper/pull/31.

This can be shorted using the `wrap` method. Documentation was missing, so I submitted https://github.com/scotteh/php-dom-wrapper/pull/31.

I think it's better to use $doc->createElement() instead of this, to get an element that is associated with the right document directly, which allows making modifications to it directly (I had problems with e.g.addClass() on directly-created DOMWrap\Element instances, even after inserting them into a document, possibly this results in a clone being made into the right document).

Another advantage of using createElement is that you do not needlessly expose the DOMWrap\Element class name (you just automatically get the right class based on the document you're using).

I think it's better to use `$doc->createElement()` instead of this, to get an element that is associated with the right document directly, which allows making modifications to it directly (I had problems with e.g.`addClass()` on directly-created `DOMWrap\Element` instances, even after inserting them into a document, possibly this results in a clone being made into the right document). Another advantage of using `createElement` is that you do not needlessly expose the `DOMWrap\Element` class name (you just automatically get the right class based on the document you're using).

system/wymeditor/lang/en.js

I'm still unsure what "cc" means here. If it's intended to be "Creative Commons", then this might actualy be misleading, since just "CC" is not specific enough. I think we should just use "e.g." here, since it's intended as a suggestion? Or actually, maybe "license: author (date)" was the intention and is actually helpful, since it suggests to people that a license is actually needed. Let's try that and see what feedback we get.

matthijskooijman commented

2020-10-06 12:52:31 +00:00

(Migrated from github.com)

One additional thing I found is that you now explicitly exclude any HTML inside wymeditor, which I had expected to be not needed since this should live as text (rather than HTML) inside a textarea. However, I found that we actually changed this to HTML in b6f630ee8b to actually allow the global dewikify to process this. However, this results in actual (non-escaped) HTML to end up in the <textarea> tag, which is not technically correct HTML (though it seems browsers will allow it in practice).

Also note that this editor starts out as an <editor> tag and is later transformed into a textarea tag by this bit of code. This makes the current code in this PR fragile: If it would happen to run before rather than after this transformation, the .wymeditor class would not exist yet and it would add captions inside the wymeditor too.

I think it might be better to just revert to storing text rather than HTML, but manually run dewikify (and maybe other things needed) on the HTML before converting it to text and inserting it into the editor.

I also wonder if we really need this editor tag at all, maybe it would be simpler and more transparent to just use <textarea class="wymeditor"> instead of <editor> in the forms directly? Does not really look any less readable to me either?

One additional thing I found is that you now explicitly exclude any HTML inside wymeditor, which I had expected to be not needed since this should live as text (rather than HTML) inside a textarea. However, I found that we actually changed this to HTML in b6f630ee8bb29a76691afe12b371ccd7c58a8987 to actually *allow* the global dewikify to process this. However, this results in actual (non-escaped) HTML to end up in the `<textarea>` tag, which is not technically correct HTML (though it seems browsers will allow it in practice). Also note that this editor starts out as an `<editor>` tag and is later transformed into a `textarea` tag by [this bit of code](https://github.com/PlanBCode/hypha/blob/309d86e61890afaf49a5860be7be5dae5ee29f60/system/core/editor.php#L173-L187). This makes the current code in this PR fragile: If it would happen to run *before* rather than after this transformation, the `.wymeditor` class would not exist yet and it would add captions inside the wymeditor too. I think it might be better to just revert to storing text rather than HTML, but manually run `dewikify` (and maybe other things needed) on the HTML before converting it to text and inserting it into the editor. I also wonder if we really need this `editor` tag at all, maybe it would be simpler and more transparent to just use `<textarea class="wymeditor">` instead of `<editor>` in the forms directly? Does not really look any less readable to me either?

laurensmartina (Migrated from github.com) reviewed

2020-10-06 21:29:03 +00:00

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

updated to "attribution"

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin hypha-119:hypha-119

git switch hypha-119

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master

git merge --no-ff hypha-119

git switch hypha-119

git rebase master

git switch master

git merge --ff-only hypha-119

git switch hypha-119

git rebase master

git switch master

git merge --no-ff hypha-119

git switch master

git merge --squash hypha-119

git switch master

git merge --ff-only hypha-119

git switch master

git merge hypha-119

git push origin master

Sign in to join this conversation.

No reviewers

matthijskooijman

No milestone

No project

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!344

Rows
Columns