Problem/Motivation

This has been reported many times, but always as an "oh by the way", never explicitly.

This is the problem:

Proposed resolution

Remaining tasks

User interface changes

See above.

API changes

None.

Data model changes

A bit more CSS.

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.64 KB

This patch makes us go from the problematic first screenshot to the much better second screenshot.

Wim Leers’s picture

Rerolled.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I am torn between

  • adding this entity type-specific CSS to minimize BC breaks on the one hand
  • doing the right thing and generalizing it to work for all entity fields on the other hand

Then again, I don't think it's likely many sites are overriding float and margin for entity fields and their contents. So maybe I should generalize? But then what about people embedding webforms and the likes, they could be negatively impacted.

Setting this to RTBC, and if nobody complains today, I'm just going to go ahead and commit it. We can easily refine this CSS later.

oknate’s picture

FileSize
81.56 KB

On a clean standard install and using a media embed with thumbnail formatter, there's no white space above the caption, but there's a lot of whitespace between the embed and the outline.

white space around embed

It looks unrelated though. I'll report this bug on the issue: #3037316: Show an outline around the <drupal-entity> element within CKEditor

oknate’s picture

I'm trying to manually test, and I'm not seeing the gap without the change. Can you add testing instructions?

oknate’s picture

Status: Reviewed & tested by the community » Needs review
Wim Leers’s picture

#5: this issue is specifically about the front end.

Also, you're testing with the thumbnail formatter Entity Embed Display, this issue is specifically about using the view mode Entity Embed Display.

So: make sure to test with data-entity-embed-display="view_mode:media.full".

oknate’s picture

Status: Needs review » Needs work
Issue tags: +Needs steps to reproduce

Testing with full view mode, I also don't see the issue. What theme are you using?

Another issue I'm seeing is the way you're adding the library, I'm not seeing it affect the CKEditor. It looks like it's adding it to the main page. If I add it here, then I see the css rules affecting the elements:

/**
 * Implements hook_ckeditor_css_alter().
 */
function entity_embed_ckeditor_css_alter(array &$css) {
  $css[] = drupal_get_path('module', 'system') . '/css/components/hidden.module.css';
  $css[] = drupal_get_path('module', 'entity_embed') . '/css/entity_embed.editor.css';
  +$css[] = drupal_get_path('module', 'entity_embed') . '/css/entity_embed.filter.caption.css';
}

I think this needs steps to reproduce, and I'm not convinced the library is added in a way that works, based on manual testing. Although, I recognize it must work for you. I was testing with Drupal 8.7.1.

I think when you add libraries in the ajax response, it adds them to the main page, not to the CKEditor which has its own encapsulated css world.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs steps to reproduce

What theme are you using?

Just Drupal core's default theme: Bartik.

Another issue I'm seeing is the way you're adding the library, I'm not seeing it affect the CKEditor.

That's why I said "this is about the front end" :)

Not that every screenshot I posted is taken at /node/5, not at /node/5/edit.

I think this is why you're not able to reproduce this: you're just looking in the wrong place :D

oknate’s picture

oh, no wonder, that's why your "wyswiyg" looks so different, it's not one. I'll retest.

Wim Leers’s picture

@oknate: Alright, looking forward to your response :)

oknate’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
244.64 KB
214.6 KB

Marking RTBC. The gap is gone on a standard install.

Without patch:
Without patch, gap
With patch:
with patch, no gap

  • Wim Leers committed 0965a5b on 8.x-1.x
    Issue #3059480 by Wim Leers, oknate, phenaproxima: Embedded media...

Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/css/entity_embed.filter.caption.css
@@ -0,0 +1,10 @@
+.caption .media .field,
+.caption .media .field * {
+  float: none;
+  margin: unset;
+}

Discussed with @phenaproxima.

I wanted to get his perspective on this particular CSS.

He initially was wondering if we should make it more specific. He agrees we should be only targeting media, because for example nodes might be showing more data that actually requires floats in order to get a sensible representation. Whereas for media, the common expectation (dare I say 100%?) is to have a single piece of media displayed, and so any floats and margins simply get in the way.

Wim Leers’s picture

Issue tags: +Media Initiative, +DevDaysCluj
rosinegrean’s picture

Issue tags: -DevDaysCluj +DevDaysTransylvania
Wim Leers’s picture

FYI:

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.