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

Allow linking images - editing #7330

Closed
6 of 7 tasks
jodator opened this issue May 29, 2020 · 6 comments · Fixed by #7386
Closed
6 of 7 tasks

Allow linking images - editing #7330

jodator opened this issue May 29, 2020 · 6 comments · Fixed by #7386
Assignees
Labels
package:image package:link type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@jodator
Copy link
Contributor

jodator commented May 29, 2020

📝 Provide a description of the new feature

Implement link on images editing:

  • add proper schema rules and converters to load / save link on image
    • the <a> element should be inside image's <figure> and set on <img> element
    • current converters and its utils should work with that structure (might assume first child of <figure> in the view
  • check LinkCommand behavior - it should be tested that this is possible
  • manual tests are welcome (loading data)
  • adjust all image features to new view (like image resize)
  • check how image/link behaves when plugin is not enabled
  • separate plugin in ckeditor5-link.
  • should be enabled in Essentials plugin

If you'd like to see this feature implemented, add a 👍 reaction to this post.

@jodator jodator added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:image package:link squad:blue labels May 29, 2020
@jodator jodator added this to the iteration 33 milestone May 29, 2020
@pomek
Copy link
Member

pomek commented Jun 3, 2020

  • the <a> element should be inside image's <figure> and set on <img> element

Should the following HTML work as well?

<a href="http://localhost">
	<img alt="bar" src="sample.jpg">
</a>

@jodator
Copy link
Contributor Author

jodator commented Jun 3, 2020

Should the following HTML work as well?

For upcast - sure.

We already enforce <figure> on <img> so <a> should follow that.

OTOH, I'm not sure how it will handle <a><figure><img> in the content but the output should be taken from the model anyway so if will upcast href to <image> in the model we will output <figure><a><img>.

@pomek
Copy link
Member

pomek commented Jun 3, 2020

<a><figure><img> seems to be invalid structure.

<a>
  <figure>
    <img>
    <figcaption>
      <a>...

What now? :D

@jodator
Copy link
Contributor Author

jodator commented Jun 3, 2020

What now? :D

This might need to be checked if this is possible in the HTML - how it behaves then. It looks like the outer <a> is for <img> and the inner for $text in caption.

@pomek
Copy link
Member

pomek commented Jun 4, 2020

check how image/link behaves when plugin is not enabled

Links are lost. For me, it sounds good.

@pomek
Copy link
Member

pomek commented Jun 4, 2020

What now? :D

This might need to be checked if this is possible in the HTML - how it behaves then. It looks like the outer <a> is for <img> and the inner for $text in caption.

Nested <a> elements are invalid. I guess we're safe if we assume that a link wraps an <img> element.

jodator added a commit that referenced this issue Jun 10, 2020
Feature (link): Introduced the linking images feature. Closes #7330.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image package:link type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants