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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

laurensmartina
Copy link
Contributor

Fixes #119

Copy link
Contributor

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
system/core/pages.php Outdated Show resolved Hide resolved
@laurensmartina laurensmartina force-pushed the hypha-119 branch 3 times, most recently from 7d206a8 to a26df84 Compare October 5, 2020 21:28
Copy link
Contributor

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
system/core/pages.php Outdated Show resolved Hide resolved
system/core/pages.php Outdated Show resolved Hide resolved
system/core/pages.php Outdated Show resolved Hide resolved
system/wymeditor/lang/en.js Outdated Show resolved Hide resolved
@matthijskooijman
Copy link
Contributor

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 b6f630e 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?

Wraps images with title or data-attribution attribute
in a figure element.
Places the title in a figcaption element.
Places the data-attribution in a small element.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement caption and meta-information for images
2 participants