Clean up "captions" component in Bartik

added file comment

couldn't apply the patch, something was strange. fixed it and uploaded the patch again.

1. There should be blank line after file comment
2. Hexas should be written in small letters

Here i changed to lowercase color values

@olmaga
Thanks for working on this task. please unassign yourself, so everybody knows that this is ready for review and not reviewed by you :)

1. We should have empty line after file-comment.
2. I tried to make blockquote to caption, but didn't manage to do it. So from my point of view that part of styling is unnecessary.

3. Should we change font-size: small to em's as this is pretty much only place in Bartik where we're using keywords?

i'm working on this now

Worked on this issue at DrupalCon LA

Addressed most of the comments from #12:
1, 2, 3, & 4 - done
5 - done, but needs work since you cant't add blockquote/pre to caption with the current wysiwyg #2268941-2: Removing caption from a previously captioned image fails to remove the caption-related classes

This issue needs screenshots.

added before and after screenshots

I'm currently reviewing this

I talked with @emma.maria to get clarification on validating blockquote and pre caption styles. These currently cannot be done through the wysiwyg UI, but can be done by manually typing in the required data-caption data attribute.

Adding a caption to a blockquote:

<blockquote data-caption="I am a blockquote caption">
<p>I am a blockquote. My data-caption='something'</p>
</blockquote>

Will yield:

Adding a caption to 'pre':

<pre data-caption="I am a pre caption">
…. content ...
</pre>

Will yield:

The issue with alignment of the figurecaption element with the image varies based on the width of the viewport:

With the issue fixed:

Interdiff file attached

Shouldn't we be using HTML6HTML5's figure/figcaption? - http://www.w3.org/wiki/HTML/Elements/figcaption

In the patches I've seen they are just CSS. Am I missing something?

EDIT: No idea why I typed HTML6....

@emma.maria HTML5's figure/figcaption is available in CKEditor. Not sure where else we're seeing examples of captions in Core. Where would be a good place (other than CKEditor) to see what Core is spitting out?

#17: <3 <3 AWESOME!

#21: this does not need to be blocked on that issue at all, just create the markup manually. Disable JavaScript, create <img src="some image" data-align="center" data-caption="Caption text." />

+++ b/core/themes/bartik/css/components/captions.css
@@ -1,23 +1,28 @@
-.caption-pre > pre,
-.caption-blockquote > blockquote {
+.caption > pre,
+.caption > blockquote {

Why these changes? Look at git blame, see the issues that made it this way. This was not done on a whim.

Nice

+++ b/core/themes/bartik/css/components/captions.css
@@ -1,23 +1,29 @@
+  -webkit-box-sizing: border-box;
+  -moz-box-sizing: border-box;
+  box-sizing: border-box;

Do we need all these vendor prefixes?

1. +++ b/core/themes/bartik/css/components/captions.css
   @@ -1,23 +1,29 @@
   -  padding-top: 0.5ex;
   ...
   +.caption:not(.caption-pre) > figcaption {
   +  border-top: none;
   +}
   

   This is now just repeating what .caption > figcaption used to do generically. Why is it worth having a separate selector for this?

2. +++ b/core/themes/bartik/css/components/captions.css
   @@ -1,23 +1,29 @@
   -.caption-pre > pre,
   -.caption-blockquote > blockquote {
   +.caption > pre,
   +.caption > blockquote {
   

   And I see how this is cleaner, but what do we really gain by this change?

.caption-blockquote
If these classes are redundant, we could remove them from the mark up?

+.caption:not(.caption-pre) > figcaption {
+  border-top: none;
+}

It's not the same, this is ensuring that the caption-pre has a top border, I thought this was fixing a visual bug?

Oh, you're right, that fixes a regression in HEAD. So, disregard #28.1.

If these classes are redundant, we could remove them from the mark up?

But they're not redundant, as the use of the .caption:not(.caption-pre) > figcaption selector shows. Until CSS has a :has() selector or something like that, we'd need this, unfortunately :(

If the captioned image width is greater than the screen width it will overhang. This does not happen with uncaptioned images. This was observed in Bartik 8.0.1.

At small browser widths the image overhangs the caption.

Fixed the issue by using CSS 3's calc().
The cause of the problem was the padding of image.
Set the max-width to 100% and displayed the .caption as block.
Used !important to override styles of other css files.

The first two image files reflect the changes.

sorry for previous patch
Uploaded new one

1. Looks like you've changed some of the coding standards while modifying the file. Please revert them.
- The file comment at the top of the file.
- Using capital letters for color codes etc.

2. I'm not a CSS expert but it'll be better if you could avoid the use of !important

Isn't this issue supposed to be solely about cleaning up the component, and not about fixing bugs? For fixing the bug, we already have #2568597: [upstream][Firefox] The CSS used for Filter/Editor captions is not responsive.

@wim This patch contains the cleaned up CSS and also fixes the bugs.

Your interdiff is the entire patch. It's impossible to see what you've changed.

And in #44 you're just repeating what I say is the problem. That's not an answer.

My mistake. I misread. This issue is about beautifying the code, not about beautifying what the code does.

I created the new patch to make sure that the css guidelines are followed and removed all the code fixes that were done on the previous patch number 44

#50 I have review the patch, apply successfully and working fine. I have attached After_patch image and Before_patch images.
I test this patch in Firefox and Chrome browser. It's Working fine.
But in captions.css file, I have found ex instated of em.
According to #12 comments and #14 patch changes, these need to be change.
So I have made these change and add patch.

Reroll the patch for 9.1.x because #52 patch is not working with 9.

Tested #60, its working fine on 9.1

Tested #60, its also working fine on 9.2-dev.

For more info please review the attachment.

Thanks!

1. +++ b/core/themes/bartik/css/components/captions.css
   @@ -3,21 +3,21 @@
   -.caption-pre > pre,
   -.caption-blockquote > blockquote {
   +.caption > pre,
   +.caption > blockquote {
   

   Why are we changing this selector?

2. We should update the @file documentation to match our current coding standards.

Providing updated version of patch #66 as per comment #65, Please review the patch

Applied patch #66 successfully. Works fine

Added patch for the 9.4.x version.

Applied patch #70 is not working.
Why are we changing the class, font-size and padding?
We can add box-sizing to fixed this.
Example:-

.caption > * {
 + box-sizing: border-box;
}

Applied patch #70 is not working.
Attached reroll patch against Drupal 9.5.x.

Updating patch after Fixing failed test cases.

Applied patch in #74 in 9.5.x-dev. This patch fixes the issue. Adding screenshots

Move this ticket to RTBC.

The last submitted patch, #74, passed testing. So, move this ticket to RTBC.

This is now a contrib theme: https://www.drupal.org/project/bartik

Hi @rpayanm

Confirmed resolved the alignment issue of the caption below the image when I applied the MR !1 to the Bartik theme against 1.0.2 version and with the Drupal core version 9.5.6.

Moving now to RTBC and
Please see the attached screenshots for your reference.