Quotations are missing for the HTML <blockquote> Element in Claro theme.

#3 Please review the patch

I have applied the patch #3. The issue is fixed and i can see the

Element. Adding before and after patch screenshots for reference.

Patch works as intended. Adding evidence as well.

- Created an article in source:

<blockquote>
<p>This should be inside a blockquote</p>
</blockquote> 

Before patch:

After patch:

Moving to RTBC!

Patch #3 applied cleanly and working as expected , sharing screenshot for reference.

Successfully applied patch
Thanks

Thanks for the issue, the patch, and all the reviews!

@radheymkumar and @Madhu kumar -- we've already had multiple folks post before/after screenshots, so we don't need more of those. We also know the patch applies cleanly thanks to the d.o test bot. Instead of duplicating the work other folks have done, it'd be more helpful to either do a careful code review, or find another issue that Needs screenshots and test/review that, instead. Removing issue credit for now, although if you meaningfully contribute here, someone can always restore the credit.

Meanwhile, some concerns on the implementation from #3. I'm only commenting on the .pcss files, since the .css ones are auto-generated from those.

1. +++ b/core/themes/claro/css/base/elements.pcss.css
   @@ -109,8 +109,29 @@ dl dl {
   -  margin: 1em 40px;
   +  margin: 1.5em 0.625em;
   +  padding: 0.5em 1em;
   

   I don't see any discussion of making such a large change to the top/bottom spacing as this. This is doubling the spacing. Not sure that's necessary or advisable.

2. +++ b/core/themes/claro/css/base/elements.pcss.css
   @@ -109,8 +109,29 @@ dl dl {
   +  font-style: italic;
   

   I'm not sure we want the entire quote to become italicized. This would be especially hard to read for a large quote, and would mask <em> or <cite> tags within the quote.

3. +++ b/core/themes/claro/css/base/elements.pcss.css
   @@ -109,8 +109,29 @@ dl dl {
   +  &::after,
   +  &::before {
   ...
   +  &::before {
   

   Seems a little weird to define 6 properties for both the ::after and ::before selectors, then override half of them for the ::before case. I wonder if there's a cleaner/simpler way to express this.

   Meanwhile, a lot of <blockquote> styles in the wild only use the quotes for the ::before case, anyway. So maybe the simplest solution is a design with only the opening quote marks?

4. +++ b/core/themes/claro/css/base/elements.pcss.css
   @@ -109,8 +109,29 @@ dl dl {
   +    font-family: "Times New Roman", Times, serif;
   

   Seems really weird to introduce a whole new font-family to Claro just for these quote marks. Seems we should fall-back to whatever font-family is already in use.

5. +++ b/core/themes/claro/css/base/elements.pcss.css
   @@ -109,8 +109,29 @@ dl dl {
   +
   

   Doesn't look like this file puts newlines between the rules, so we should probably leave this one out.

6. +++ b/core/themes/claro/css/components/form--text.pcss.css
   @@ -122,6 +122,10 @@ _:-ms-fullscreen,
   +.quickedit-toolgroup.wysiwyg-main form label {
   +  padding-right: 0.8rem;
   +}
   +
   

   This seems out of scope and unrelated. We should remove this from the patch.

More generally, is there not an existing Claro <blockquote>design we can implement here? I feel weird commenting on design choices, but I don't see any references to Claro's prior art in this space, so I'm doing the bikeshed thing and adding my $0.02 of unsolicited design opinions, instead. ;)

Finally, @all: if you want to use html tags in your comments, don't forget to wrap them in <code> tags, or else d.o will try to render them directly. ;)

<blockquote> is just a tag for reference.

This is an actual quote

;)

Thanks,
-Derek

Yeah, drat, I see nothing about quotes at https://www.drupal.org/docs/8/core/themes/claro-theme/design

Tagging for "Needs design" so we can get the Claro maintainers to come up with a goal here that can then be implemented.

Also, adding a real issue summary.

Thanks,
-Derek

Here are two quick design proposals:

I love both of them, thanks @saschaeggi!! If I had to choose though, I'd go for the classical quoted one so we can leave all visual weight & decoration to UI elements that might need vertical lines like groups or tabs.

Added a patch based on the comment from @ckrina. Attached the screenshot below.

It's a new patch so there is no interdiff.

New to patch to add RTL support.

Thank @imalabya! We discussed @saschaeggi 's design on Slack and agreed on not using bold for the quote itself to visually empathize the quote and use bigger text instead. This way we can still use bold text on a quote.

Here's the design Sascha created:

Sorry, forgot to change this to Needs work based on the new design.

#16 Please review the patch

The font-size should be 1.25rem
Sorry for not providing any specs yet - I'm currently on vacation 🙈

I am working on it

Find out the changes in attachment patch files

1. +++ b/core/themes/claro/css/base/elements.css
   @@ -130,7 +130,26 @@ dl dl {
   +  margin: 1em 3.4375rem;
   

   Let's make it round to 3.5

2. +++ b/core/themes/claro/css/base/elements.css
   @@ -130,7 +130,26 @@ dl dl {
   +  float: left;
   +  margin: 0.28em 0 0 -0.65em;
   

   Will need a /* LTR */ to specify that this is overwritten

3. +++ b/core/themes/claro/css/base/elements.css
   @@ -130,7 +130,26 @@ dl dl {
   +  font-family: serif;
   

   I saw it in my patch as well, this is not required

4. +++ b/core/themes/claro/css/base/elements.css
   @@ -130,7 +130,26 @@ dl dl {
   +  transform: rotateY(180deg);
   

   This will invert the quote mark in RTL

Added a patch.

Verified and tested patch #21.
Patch applied successfully and looks good to me as mentioned in comment #20

Testing Steps:
# Go to Appearance -> Set Claro theme
# Create a blockquote post in the body
# Save it

Expected Results:
# Quotations should have appeared for the HTML blockquote Element in the Claro theme

Actual Results:
# Quotations are missing for the HTML blockquote Element in the Claro theme

Please refer attached screenshots.
Can be a move to +1RTBC.

Some little clean-up

Not sure what to do with font-family for open-quote icon, coz in different browsers this icon can look differently.

For previous comment try to open it in FF browser for example

Also we can't use `initial` value https://caniuse.com/css-initial-value

Ideas ?

Tagging for DrupalCon Europe2021 and novice. Thanks

Added Steps to validate/reproduce to summary field.

Testing Steps:

1. Install Drupal core 9.2.0
2. Go to Appearance -> Set Claro theme
3. Create a blockquote post in the body
4. Save it
5. View the post with the Claro theme.

Expected Results:

1. Quotations should have appeared for the HTML blockquote Element in the Claro theme

Actual Results:

1. Quotations are showing for the HTML blockquote Element in the Claro theme

Working as expected. Please find the attached screenshot for reference.
Hence this ticket can be moved to RTBC.

+++ b/core/themes/claro/css/base/elements.pcss.css
@@ -109,7 +109,23 @@ dl dl {
+  font-family: serif;

I think we should remove this. I don't see this font in the designs proposed by @saschaeggi.

Addressed #28

Moved to Needs Review again for reviewers to review.

#29 fixes #28 (which I also pointed out at #8.4).

Overall, the patch now looks good. This seems slightly weird with 4 significant digits:

+ margin-top: 0.6875rem;

But I guess that's fine. No real harm, I suppose.

Back to RTBC.

Thanks everyone!
-Derek

Hrm, not so fast. Now I'm seeing this:

Yeah, if we want to achieve what @saschaeggi posted in #10 and confirmed by @ckrina in #14, we need a font-family in here. So maybe the version from #3 is actually better. ;) I don't see the problem that just using font-family: serif; gives different results in different browsers (I tried FF, Chrome and Safari), but it's all on my mac, so that's probably not the best test.

I think maybe #23 is actually the right patch here now. But maybe @lauriii can re-confirm his thoughts in light of these last few comments...

For reference, here's what I'm seeing w/ #23 on Chrome, Firefox + Safari:

Chrome

Firefox

Safari

I can't tell the difference. 😉

Tentatively back to RTBC for #23. Let's see what core committers think now.

@ckrina @saschaeggi looking at the designs my initial reaction is the with the quote and margin and the font size increase that's a lot of visual emphasis. Are we sure that we should increase the font size as well? I was wondering if the content had multiple quotes and headings whether or not the increase in size would be necessary. I discussed this @lauriii and we agreed to ask this before proceeding with the issue. In isolation it does look nice.

I also think the font is too big, also is it really a good idea to leave no <"> at the end of the quotation? I think there should be something to show the end of it, like a slightly different background color or maybe the <"> itself so it's clearer for the users. Thoughts?

I added the close quote and it looks like this when i use paragraph

and it looks like this when i use break line

I tried doing the background color differently but it didn't work as i wish it would so i didn't change that, all these changes i did were based on the patch on #23, not the #29, all i need now is the review and test this on other languages and let me know if the font-size should be different.

Alright, I've tweaked the design a bit. Reduced the spacings, also decreased the font-size so that it's only marginally bigger.
Also you'll find all the specifications in Figma now:
https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system...

I still think there should be a close-quote (on a blockquote::after) because it looks incomplete/unfinished, like when you don't put a period at the end of a sentence.

Thanks @saschaeggi! This will accommodate longer quotes as @alexpott, @lauriii and @vicheldt were suggesting. If a user reaches a limit were it looks weird with so many quotes with this design I'd say it's more a content problem rather than a design one. So +1 to this.

@vicheldt thanks for your suggestion. We chose going just with the first quote as a visual indicator instead of 2 in purpose, and use it together with the extra space to indicate what is a quote without loading it too much with visual information. So it's a classic design patter, we haven't missed it. That's why the blue quote element itself is that big: if we were going to use 2 they should go more on line with the text size and color.

Also, @saschaeggi confirmed on the design specs we should use serif as the font size.

Applied patch #13 in drupal-9.3.x-dev applied successfully
Thanks for the patch ...
For ref sharing screenshot ...

Next step here is to implement feedback from #42.

Hi! Here's a new patch addressing #42 and the new design, it was also necessary to define the close quote even if it's not used, otherwise the browser can interpret the second quote as being inside of the first one.

blockquote::after {
  content: no-close-quote;
}

Before:

After:

The font family for the quotation mark is specified as font-family: "Times New Roman", serif; as it is on the Figma, maybe a --font-family-serif variable should be added to contain this?

Visually it looks good now, thanks.
Now just needs a small code review.

Nice work @javi-er, thanks!! I totally agree with you about the need to define a variable for --font-family-serif so it doesn't get defined in other ways in the future (and in other places). I'd say let's add it as --font-family-serif: "Times New Roman", Times, serif; in the variables.pcss.css file.

As per #48, --font-family-serif: "Times New Roman", Times, serif; font family added to variables.pcss.css file, Attached an interdiff for same. Please review.

Fixed custom command failure errors in #49 by running tests locally, thanks!

Thanks for keeping this moving forward, everyone!

Re: patch #51:

+++ b/core/themes/claro/css/base/variables.pcss.css
@@ -49,6 +49,7 @@
+  --font-family-serif: "Times New Roman", times, serif;

Why lowercase times here? Isn't it a proper noun? The other usages in core use "Times". Not sure why this changed here.

Everything else looks good to my eyes. If it weren't for this, I'd RTBC.

Thanks again,
-Derek

The test bot on code-check reported the following error:

Checking core/themes/claro/css/base/variables.pcss.css

yarn run v1.22.17
$ node ./scripts/css/postcss-build.js --check --file /var/www/html/core/themes/claro/css/base/variables.pcss.css
[09:19:56] '/var/www/html/core/themes/claro/css/base/variables.pcss.css' is being checked.
Done in 1.42s.

themes/claro/css/base/variables.pcss.css
 52:43  ✖  Expected "Times" to be "times"  value-keyword-case

Do we need to change it back to uppercase times ?

Oh okay. Weird. I wonder why bartik's CSS is okay and this isn't. I guess bartik isn't using postcss so we don't notice or care. /shrug

In that case, I think this is RTBC.

Thanks!
-Derek

Adjusting credits and uploading Drupal 10 version of the patch in #51.

Committed 11fc6f0 and pushed to 10.0.x. Also committed the patch from #51 to 9.4.x. Thanks!