Currently in the Appearance section of Bartik, a live preview of a chosen color scheme is displayed as an image. This section uses Latin placeholder text, "Lorem ipsum . . . ." The problem is getting users to understand the Latin is JUST A PLACEHOLDER. Participants in a Drupal usability study at the University of Minnesota in 2011 expected it to be the live preview of the site with THEIR content. Looking at the Latin text, one participant in the study said, "What's this? French?"

In this thread from May 2011 to December 2014, contributors have narrowed in on recommending a patch to change the Latin text to repetitions of the English words "Sample text." Other suggestions have been to make the text translatable or to embed the content of the actual site as an iFrame. Placing "sample text" is the most simple of the solutions and would look like this:

Sample text

Block of text (re-arrange sentences so line lengths don't repeat)
This is a block of example content. This content is here to demonstrate what actual content would look like in the color scheme you configure. This is an example link, is is here to demonstrate how background colors you configure may affect the readability of links. This content is here to demonstrate what actual content would look like in the color scheme you configure. This is an example link, is is here to demonstrate how background colors you configure may affect the readability of links. This is a block of example content.

CommentFileSizeAuthor
#263 reroll_diff_260-263.txt11.56 KBravi.shankar
#263 1167366-263.patch8.17 KBravi.shankar
#260 interdiff_253-260.txt3.85 KBravi.shankar
#260 1167366-260.patch14.91 KBravi.shankar
#258 Screenshot from 2022-06-09 19-05-12.png80.57 KBShiv Yadav
#255 PreviewWithNewText.png153.84 KBJohnny Santos
#255 PreviewWithNewText.png153.84 KBJohnny Santos
#253 diff_1167366_251-253.txt817 bytesandregp
#253 1167366-253.patch14.94 KBandregp
#251 diff_1167366_238-250.txt3.56 KBandregp
#251 1167366-250.patch14.85 KBandregp
#248 interdiff_245-248.patch641 bytesravi.shankar
#248 1167366-248.patch14.64 KBravi.shankar
#245 diff_1167366_243-245.txt428 bytesandregp
#245 1167366-245.patch14.64 KBandregp
#243 interdiff_241-243.txt893 bytesranjith_kumar_k_u
#243 1167366-243.patch14.57 KBranjith_kumar_k_u
#241 diff_1167366_238-241.txt1.33 KBandregp
#241 diff_1167366_239-241.txt1.58 KBandregp
#241 1167366-241.patch14.61 KBandregp
#239 diff_1167366_238-239.txt1.6 KBandregp
#239 1167366-239.patch14.54 KBandregp
#238 1167366-238.patch14.52 KBravi.shankar
#231 1167366-231.patch14.48 KBnarendra.rajwar27
#231 interdiff_1167366_215-231.txt1.32 KBnarendra.rajwar27
#229 1167366-229.patch14.64 KBnarendra.rajwar27
#229 interdiff_1167366_215-229.txt899 bytesnarendra.rajwar27
#227 1167366-227.patch14.41 KBnarendra.rajwar27
#227 interdiff_1167366_215-227.txt715 bytesnarendra.rajwar27
#223 1167366-223.patch14.44 KBpradeepjha
#215 interdiff_198_215.txt3.1 KBjphelan
#215 color-replace-lorem-ipsum-example-text-1167366-215-D8.patch14.17 KBjphelan
#212 interdiff_198_209.txt2.66 KBjphelan
#211 color-replace-lorem-ipsum-example-text-1167366-209-D8.patch14.2 KBjphelan
#208 color-replace-lorem-ipsum-example-text-1167366-208-D8.patch7.08 KBjphelan
#180 1167366-180.patch14.13 KBAda Hernandez
#180 interdiff.txt2.45 KBAda Hernandez
#177 1167366-177.patch14.25 KBrakesh.gectcr
#177 interdiff-1167366-175-177.txt1.55 KBrakesh.gectcr
#175 interdiff-1167366-171-175.txt1.33 KBrakesh.gectcr
#175 1167366-175.patch14.22 KBrakesh.gectcr
#172 interdiff-1167366-156-172.txt2.79 KBrakesh.gectcr
#172 1167366-172.patch14.36 KBrakesh.gectcr
#158 interdiff-1167366-155-156.txt7.64 KBleolandotan
#157 replace_color_module_template_lipsum_text-1167366-156.patch14.08 KBc.nish2k3
#155 interdiff-1167366-148-155.txt9.12 KBleolandotan
#155 replace_color_module_template_lipsum_text-1167366-155.patch14.17 KBleolandotan
#151 Screenshot 2017-04-28 12.53.33.png286.23 KBmikeohara
#148 interdiff-1167366-146-148.txt691 bytesrakesh.gectcr
#148 1167366-148.patch14.27 KBrakesh.gectcr
#146 interdiff-1167366-144-146.txt4.52 KBrakesh.gectcr
#146 1167366-146.patch14.26 KBrakesh.gectcr
#146 issues-1167366-ml.png429.43 KBrakesh.gectcr
#146 issue-1167366-en.png273.28 KBrakesh.gectcr
#144 interdiff-1167366-141-144.txt2.92 KBrakesh.gectcr
#144 1167366-144.patch10.23 KBrakesh.gectcr
#144 1167366-144.patch10.23 KBrakesh.gectcr
#141 interdiff-1167366-139-141.txt4.04 KBwturrell
#141 1167366-141.patch8.9 KBwturrell
#139 interdiff-1167366-134-139.txt0 bytesrakesh.gectcr
#139 interdiff-1167366-131-139.txt2.01 KBrakesh.gectcr
#139 1167366-139.patch8.6 KBrakesh.gectcr
#134 interdiff-132-134.txt1.08 KBjofitz
#134 1167366-134.patch9.66 KBjofitz
#132 interdiff-130-131.txt4.25 KBtameeshb
#132 interdiff-127-131.txt5.42 KBtameeshb
#132 1167366-131.patch9.66 KBtameeshb
#131 1167366-130.patch9.69 KBtameeshb
#131 interdiff-127-130.txt1.61 KBtameeshb
#127 interdiff-1167366-125-127.txt2.81 KBswarad07
#127 1167366-127.patch9.7 KBswarad07
#125 interdiff-1167366-118-125.txt5.19 KBwturrell
#125 1167366-125.patch8.66 KBwturrell
#121 after-1167366-118.png131.03 KBaviseksingh
#121 before-1167366-118.png100.89 KBaviseksingh
#118 1167366-118.patch8.61 KBwturrell
#116 1167366-116.patch13.28 KBwturrell
#116 interdiff-1167366-114-116.txt2.68 KBwturrell
#115 before.png94.86 KBtameeshb
#115 after.png117.21 KBtameeshb
#114 interdiff-109-114.txt5.54 KBtameeshb
#114 1167366-114.patch13.47 KBtameeshb
#109 1167366-109.patch8.79 KBdroplet
#109 interdiff.patch1.09 KBdroplet
#105 interdiff-1167366-102-105.txt1.1 KBrakesh.gectcr
#105 1167366-105.patch8.91 KBrakesh.gectcr
#102 interdiff-1167366-92-102.txt1023 bytesrakesh.gectcr
#102 1167366-102.patch8.9 KBrakesh.gectcr
#95 interdiff-1167366-92-95.txt843 bytestameeshb
#95 1167366-95.patch9.78 KBtameeshb
#92 interdiff-1167366-80-92.txt8.29 KBrakesh.gectcr
#92 1167366-92.patch8.58 KBrakesh.gectcr
#80 Issue1167366-80.patch4.83 KBtameeshb
#78 node1167366-78.patch4.84 KBtameeshb
#67 Issue1167366-67.patch4.91 KBtameeshb
#64 Issue1167366-64.patch4.91 KBtameeshb
#58 Correcting-Indentation-1167366-58.patch737 bytestameeshb
#58 56-and-Correcting-Indentation-1167366-58.patch1.9 KBtameeshb
#46 After_apply_patch.png97.68 KBbrahmjeet789
#43 Before_apply_patch.png79.87 KBbrahmjeet789
#42 1167366-42.patch3.71 KBtameeshb
#35 users-do-not-understand-lorem-ipsum-under-color-scheme-preview-1167366-35.patch3.6 KBtameeshb
#28 2014-12-31 17-44-14.jpg115.04 KBSteffenR
#184 Bartik drupal 8.4.x 1167366 180 patch--English.png233.06 KBgreggmarshall
#184 Bartik drupal 8.4.x 1167366 180 patch--French.png242.37 KBgreggmarshall
#188 1167366-188.patch14.19 KBharsha012
#188 interdiff-180-188.txt5.5 KBharsha012
#195 1167366-195.patch14.29 KBrakesh.gectcr
#195 interdiff-1167366-188-195.txt1.95 KBrakesh.gectcr
#198 interdiff-1167366-195-198.txt844 bytesAda Hernandez
#198 1167366-198.patch14.22 KBAda Hernandez
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yoroy’s picture

Version: 7.x-dev » 8.x-dev
Component: user interface text » Bartik theme

Has to be fixed in head first. I think this is a Bartik specific feature.

JohnAlbin’s picture

Title: Users do not understand "Lorem Ipsum" under Appearance Preview » Users do not understand "Lorem Ipsum" under Color Scheme preview
Status: Active » Postponed (maintainer needs more info)

Its the theme setting's color scheme preview. It is not, and cannot, be a live preview.

The reason it can't be a "live preview" with real content is because the theme needs to show the colors for regions that may not actually have any content assigned to it on the live site.

For example, the sidebar and footer regions may not have any regions. But the color scheme settings clearly show sidebar and footer color options. Having settings for a completely missing part of the theme would be a bigger problem then what this issue is describing.

Which makes me ask… What exactly is the problem? A user said "What is this? French?" How did that prevent them from completely their task? Understanding the full problem is necessary to fix anything.

yoroy’s picture

Status: Postponed (maintainer needs more info) » Active

Lets forget the live preview part for now.

The loremipsum might not prevent them from completing a task (which would make this critical, which it isn't), but it throws people off. It causes unnecessary mental friction. I've seen it happen a couple of times now in other usability tests too (with lorem ipsum used for filler content). People start reading it and wonder what the hell they are looking at. So we're distracting them instead of helping them complete the task at hand. That's the problem.

Replacing it with 'This is sample text. This is sample text. This is sample text. This is sample text. This is sample text. This is sample text. This is sample text. This is sample text. This is sample text. This is sample text. This is sample text. This is sample text." would probably a good start.

Jeff Burnz’s picture

There's a problem with making this English - the preview file is not php, its html, so is not parsed, which means we can't translate it.

dcmistry’s picture

+1 to yoroy

joachim’s picture

-1.

This is just silly -- there are always going to be new concepts to learn, and lorem ipsum is an established way of doing this.

droplet’s picture

-1
how many users not take part in usability tests trying to read & understanding it ?? I'm not. when you asking them question, they trying to scan and found the answers, it's diff than common usage

Jeff Burnz’s picture

> one participant... "What's this? French?"

Despite the fact that Lorem Ipsum does indeed have known issues, I am not really convinced this needs to change - making this English doesn't help those who do not speak English, to them it may as well be in French.

To make this translatable could be a big job, perhaps the current approach is not the perfect solution, but is an acceptable satisficer.

In short I think we got bigger fish to fry.

longwave’s picture

Another -1 to changing this. Lorem ipsum is the standard for dummy text. If this is to remain open, it should be minor at best.

Bojhan’s picture

Priority: Normal » Minor

Sorry, but I completely disagree with the premise of some, that everyone understands or knows what Lorem Ipsum is. It being a standard does not make it understandable, and for whom is it a standard; graphic designers/typographer and web developers?

It should be fine if we make it translatable, and only use "This is sample text". I do not get other than some technical constraints, that would not be a good fix.

We do have bigger fish to fry, and a actual live preview could be the best solution (But as I understand not easy). But this can be a small fix - lets not forget we have a whole part of the community who works on this.

That in mind, it is a small issue - it did not stop the participant, just confused the participant for a second (making it minor).

TR’s picture

Perhaps the usability problem is that it's called "Preview" instead of something like "Example".

"Preview" implies the image is what your site will look like; when it's clearly not (contains Lorem etc.) users get confused.

A simple change in what we call this "preview image" should avoid any confusion.

Jeff Burnz’s picture

Status: Active » Postponed

We should speak with Gàbor about it, could be quite tricky to fix in D8, right now we don't really know because the Internationalization initiative is just getting started and is blocked somewhat by Configuration Management.

If its easy (later on), then great - but right now I can't see that its possible to fix this so we should postpone it.

joachim’s picture

> Sorry, but I completely disagree with the premise of some, that everyone understands or knows what Lorem Ipsum is.

I'm not saying everyone understands it; I'm saying it's really not that hard to deduce what it's for.

dcmistry’s picture

I agree with Bojhan and Roy that we need to change this.

Joachim: I'm saying it's really not that hard to deduce what it's for.
It is NOT that hard but it is not easy. It seems like we are just adding cognitive load to the user when not required. I do understand the technical concerns but I am sure there is a way around.

mrfelton’s picture

I come from a graphic design background and so to me, Lorum Ipsum is a well known device, as it is most graphic designers. Many of our users do not come from the same design background, and in their case Lorum Ipsum simply looks like Latin text - which it is. I have been faced with confused clients in the past after presenting them with a mockup for their website/poster/magazine full of latin placeholder text, I suspect that a significant proportion of our users may experience a similar confusion when looking at this 'preview'.

I agree with 2 of the suggestions made in this thread.

1) 'Preview' is a bad name for this feature as i is not a preview, it's an example. So lets rename it to something like 'Example'

2) Lorum Ipsum may confuse a large proportion of users, so this text should be changed to something that is unlikely to confuse people - preferably something in their own language.

I think we need some input from the internationalisation as to the possibilites of making this text translatable.

Bojhan’s picture

Status: Postponed » Active
couturier’s picture

Why not compose several paragraphs using international words that are similar among many languages? For example:

A chauffeur took us in an automobile from our hotel. We listened to the radio and stopped at a bank and a restaurant. Then we went to a ballet at the theatre. A piano and violin played music for the program. On the way back to the hotel, we stopped at a cafe for chocolate and coffee.

Algebra, arithmetic, biology, chemistry, geography, geology, geometry, mathematics, physics, physiology, psychology and zoology are university programs. January, February, March, April, May, June, July, August, September, October, November and December are on the calendar. One, two, three, four, five, six, seven, eight, nine and ten are universal words.

Of course, that is just a quick example I threw together using words from an international word list on Wikipedia. If you like, I am willing to try to compose a paragraph of greater substance. I was surprised to see this issue, because I never questioned the use of Lorem Ipsum. However, I researched several other posts online just now and saw that Latin text is a problem for many other people who are not from design backgrounds. My full-time job is in apparel design, and I often encounter clients who cannot imagine even the smallest detail to be different than from what they see in a sketch or sample. So, I would be in favor of offering people as much support in the appearance settings as possible for replicating a real page of text. Using international words might help minimize the confusion until translation support becomes available.

dcmistry’s picture

@couturier I like your perspective on this especially the first one "the chauffeur...." It might be interesting to see how that works. Any research articles that suggest this works better?

We take "Lorem Ipsum" as second nature that it is placeholder text but it continues to amaze me how much of issue it creates.

couturier’s picture

The articles I saw just suggested replacing the Latin with real, meaningful text. I'm not aware of any research for international words as a replacement for multi-lingual audiences. That was just my initial response from knowing that words overlap in several languages I've studied. Since we are targeting worldwide users for that page, not just one client, "meaningful" depends on how well the words are understood among all languages, as already pointed out here in others' comments.

Since Drupal is a leader, anything different on that page will attract industry attention. The challenge in using international words is making the text sound serious enough. I've also been thinking, would we get more complaints if we abandoned Lorem Ipsum than if we kept it? Probably the majority of web designers recognize it. Anyone who does not know can search the Internet to learn its use. Should we expect web designers to be educated in the use of Latin filler? I'm not saying we should; I'm just considering the future of web design standards, since Drupal sets precedent.

emma.maria’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes
Status: Active » Postponed

Postponing as people agreed it is a valid issue but no one has suggested any further with what to do about it. Feel free to put this active again if you have any ideas.

couturier’s picture

Status: Postponed » Needs review

emma.maria are you on the committee to be able to make this change in Drupal 8? This thread contains demonstrations of several suggestions for change, but at this point my favorite goes to the original poster's first idea, "Sample text." I've been interested to see what would happen with this issue as we near the release of D8. A new wave of users is expected to be drawn to Drupal with the launch of D8, and "sample text" written in English would cause less confusion for younger web developers and international users than would the historic Latin text. For example:

Sample text

Sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text sample text.

emma.maria’s picture

Status: Needs review » Active

Hi @couturier we can certainly create a patch and get people's opinions on it. It will help kick things off and get more people joining in. We still need an agreement from a good few people in the community.

Setting to active as there are no patches to review.

Jeff Burnz’s picture

"sample text" looks good, the old latin is horrible I agree.

Is it plausible to jam it in there with JS, use Drupal.t so its translatable?

LewisNyman’s picture

hm, it's basically a flat html file in an iframe isn't it?

couturier’s picture

Issue summary: View changes
Jeff Burnz’s picture

Theres no iframe, it just does a file_get_contents() on the html file and writes it into a variable. JS loads already, sticking the text in JS should be trivial (instead of having it in the html). Whats important about the html is not the text, but the structure and classes used by the JS to color bits of it and stick in a logo etc.

emma.maria’s picture

Issue summary: View changes
SteffenR’s picture

FileSize
115.04 KB

I just stumpled upon this issue. If we care about the main content we should also take care about the menu, sidebar and Footer Content.

All content can be changed in the file core/themes/bartik/color/preview.html and the /core/themes/bartik/color/preview.js (in case of making all the text translatable via "JS injected text" as mentioned above.

SteffenR

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tkoleary’s picture

Issue tags: +sprint, +Novice
tameeshb’s picture

@tkoleary Is this issue to be worked upon and are the changes to be made?
If yes, may I work on this as my first issue fix?

tkoleary’s picture

@tameeshb Certainly!

Bojhan’s picture

Is this part of the theme?

tameeshb’s picture

With grep:
/v/w/h/drupal8.3> grep -r "lorem" ./
File:
./core/themes/bartik/color/preview.html:<h1 class="color-preview-page-title">Lorem ipsum dolor</h1>

I've replaced

  • most texts with "sample text"
  • tabs with: "Tab 2", "Tab 3"...
  • Footer links with "Link 1", "Link2"

Please suggest corrections in changes.

tameeshb’s picture

Status: Active » Needs review

Please Review and suggest changes.

tameeshb’s picture

@Bojhan #34 Yes this is a part of the default theme "Bartik"'s color scheme preview and the .html file is also in the theme's directory.

tameeshb’s picture

Assigned: Unassigned » tameeshb
tkoleary’s picture

@tameeshb

While the "sample text" is certainly more explicit about what it is and why it is there, it is not really doing the job of representing real content if it is simply the same two words repeated. I would suggest:

Heading
This is an example heading

Block of text (re-arrange sentences so line lengths don't repeat)
This is a block of example content. This content is here to demonstrate what actual content would look like in the color scheme you configure. This is an example link, is is here to demonstrate how background colors you configure may affect the readability of links. This content is here to demonstrate what actual content would look like in the color scheme you configure. This is an example link, is is here to demonstrate how background colors you configure may affect the readability of links. This is a block of example content.

Tabs (It's unlikely tabs will contain numerals)
The first tab, Another tab, The last tab

Footer links
One footer link, Another footer link, A third footer link

yoroy’s picture

From #23:

Is it plausible to jam it in there with JS, use Drupal.t so its translatable?

Translation challenges is at least part of the reason for why it is latin now. All english variations should take translating the sample text into account.

tkoleary’s picture

Is it plausible to jam it in there with JS, use Drupal.t so its translatable?

Should be a twig file, then wrap the strings in %trans, I think.

tameeshb’s picture

@tkoleary Yes I think this is a better idea.

brahmjeet789’s picture

FileSize
79.87 KB

@tameeshb i have reviewed your patch and it is working fine. The lorem ipsum content has replaced by the text that @tkoleary provided in the #39 comment. I also attached the screen shot for same

brahmjeet789’s picture

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

Alright then @brahmjeet789 @tkoleary What's to be done next?

brahmjeet789’s picture

FileSize
97.68 KB

oops forgot to attach screenshot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 42: 1167366-42.patch, failed testing.

cilefen’s picture

Issue summary: View changes

What happened to the translatability?

tameeshb’s picture

Why does this now say that it failed the testing when before it had passed the tests?
Build version is still same I suppose.

tameeshb’s picture

It's passed again

tameeshb’s picture

Status: Needs work » Reviewed & tested by the community
tameeshb’s picture

Switched back to RTBC after switch to "Needs work" due to failing a previous test.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

What happened to the translatability?

tkoleary’s picture

Status: Needs work » Reviewed & tested by the community

@cilefen

What happened to the translatability?

This issue is about resolving a usability problem with the sample text. Let's get this committed and open another issue for making it translatable.

cilefen’s picture

Oh, I thought it is twig... my bad.

+++ b/core/themes/bartik/color/preview.html
@@ -39,22 +40,22 @@ <h1 class="color-preview-page-title">Lorem ipsum dolor</h1>
           <div class="content">
-            Maecenas id porttitor Ut enim ad minim veniam, quis nostrudfelis.
-            Laboris nisi ut aliquip ex ea.
+          This is a block of example content. This content is here to demonstrate what actual content would look like
+          in the color scheme you configure.
           </div>

It looks like some indentation has gone missing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

core/modules/color/preview.html needs adjusting too if we're going to do this.

tameeshb’s picture

@cilefen I'll see the Indentation again and
@alexpott , alright, i'm working on it.

tameeshb’s picture

56-and-Correcting-Indentation-1167366-58.patch
Contains both fixes: Indentation fix and Patch for #56

Correcting-Indentation-1167366-58.patch
Contains only correcting indentation.

tameeshb’s picture

Issue summary: View changes
Status: Needs work » Needs review

The last submitted patch, 58: 56-and-Correcting-Indentation-1167366-58.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 58: Correcting-Indentation-1167366-58.patch, failed testing.

tameeshb’s picture

Why did this test fail? I just added a tab in the previous commit.

cilefen’s picture

You must post the entire diff against the 8.3.x branch.

tameeshb’s picture

tameeshb’s picture

Status: Needs work » Needs review
cilefen’s picture

+++ b/core/themes/bartik/color/preview.html
@@ -39,22 +40,23 @@ <h1 class="color-preview-page-title">Lorem ipsum dolor</h1>
           <div class="content">
-            Maecenas id porttitor Ut enim ad minim veniam, quis nostrudfelis.
-            Laboris nisi ut aliquip ex ea.
+          This is a block of example content. This content is here
+          to demonstrate what actual content would look like
+          in the color scheme you configure.
           </div>

Indentation is still changed in this patch.

tameeshb’s picture

cilefen’s picture

  1. +++ b/core/modules/color/preview.html
    @@ -1,7 +1,7 @@
    -    <h2>Lorem ipsum dolor</h2>
    -    <p>Sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud <a href="#">exercitation ullamco</a> laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>
    +    <h2>Example heading</h2>
    +    <p>This is a block of example content. This content is here to demonstrate what actual content would look like in the color scheme you configure. <a>This is an example link</a>, is is here to demonstrate how background colors you configure may affect the readability of links. This content is here to demonstrate what actual content would look like in the color scheme you configure.</p>
    

    Why is some text repeated in this section? It looks strange on a live site.

  2. +++ b/core/themes/bartik/color/preview.html
    @@ -15,21 +15,22 @@
    -          Sit amet, <a>consectetur adipisicing elit</a>, sed do eiusmod tempor
    -          incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis
    -          nostrud <a>exercitation ullamco</a> laboris nisi ut aliquip ex ea
    -          commodo consequat. Maecenas id porttitor Ut enim ad minim veniam, quis nostr udfelis.
    +          This is a block of example content. This content is here to demonstrate what actual content would look like
    +          in the color scheme you configure. <a>This is an example link</a>, is is here to demonstrate
    +          how background colors you configure may affect the readability of links. This content is here to demonstrate
    +          what actual content would look like in the color scheme you configure. <a>This is an example link</a>, is is here to demonstrate
    +          how background colors you configure may affect the readability of links. This is a block of example content.
    

    Why is some text ("This is a block of example content" and more) repeated in this section? It looks strange on a live site.

tameeshb’s picture

It was from as suggested in #39
Any other alternative to change?

tkoleary’s picture

@cilefen

Why is some text ("This is a block of example content" and more) repeated in this section? It looks strange on a live site.

The purpose of the repetition is to get enough text to make realistically sized paragraphs.

When you say "looks strange" can you be more specific?

cilefen’s picture

It looks strange. It looks strange. See?

cilefen’s picture

I wrote #71 on a phone. The reasoning behind this issue is that Lorem Ipsum is confusing to some site admins, so we would like to communicate in the theme preview plainly, and in a living language. To me, it follows naturally that what is written should not be confusing.

(edited)

tameeshb’s picture

But because, we are writing it in English, and the sysAdmin would realise this is sample text so it shouldn't be confusing if they know this is sample text.

tameeshb’s picture

Any inputs on what should be done on this?

cilefen’s picture

I would rather not see a repeating sentence if that can be avoided. It is not a big deal if there is consensus.

Fake Latin: confusing
Repeating English: somewhat confusing (?)
Plain English: not confusing

tameeshb’s picture

@cilefen Will the following work?
This is a block of example content. This content is here to demonstrate what actual content would look like in the color scheme you configure. This is an example link, is is here to demonstrate how background colors you configure may affect the readability of links. This is another sentence to demonstrate what actual content would look like in the color scheme you configure. This is another example link. This is the end of this block of example content.

cilefen’s picture

I'm on a phone, but, yes that sounds better! Watch out: there is an "is is" typo in that sample text. Roll a patch and lets get this done.

I would like a follow up issue opened to make this stuff translatable.

tameeshb’s picture

Patch with text modifications from #76
Please review & commit! :)

Status: Needs review » Needs work

The last submitted patch, 78: node1167366-78.patch, failed testing.

tameeshb’s picture

tameeshb’s picture

Status: Needs work » Needs review

Please review patch

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ifrik’s picture

Tameeshb,
can you unassign yourself? Otherwise nobody else will pick this up to review it.

aroua_safa’s picture

I'm going to review it

aroua_safa’s picture

Status: Needs review » Reviewed & tested by the community

I made a review on 8.4.x versions and confirm that the patch work.

tameeshb’s picture

Assigned: tameeshb » Unassigned
webchick’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs review

Nice! MUCH better!

However, I'm missing a plan on how we're going to translate this text if it moves away from Latin gibberish, which does not need to be translated? Is it in one of the comments that I just missed?

cilefen’s picture

Exactly #53

xjm’s picture

Version: 8.3.x-dev » 8.4.x-dev
Gábor Hojtsy’s picture

Status: Needs review » Needs work

All the way from 2011: :)

There's a problem with making this English - the preview file is not php, its html, so is not parsed, which means we can't translate it.

So why not make it a Drupal controller outputting that verbatim HTML with the translated pieces?

rakesh.gectcr’s picture

Assigned: Unassigned » rakesh.gectcr

I do agree Gabor for the same. would love to give it try

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
8.58 KB
8.29 KB

Thanks @Gábor Hojtsy, @manojapare and @lauriii for the support.

Gábor Hojtsy’s picture

That looks fine to me in resolving the translatability concerns. :) If this works well (I don't have time now to test), then it should be ready to get in :)

Status: Needs review » Needs work

The last submitted patch, 92: 1167366-92.patch, failed testing.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
9.78 KB
843 bytes

Removed javascript alert

rakesh.gectcr’s picture

Assigned: rakesh.gectcr » Unassigned

@tameeshb Thanks for the patch, Ones you working, Kindly assign to yourself and work or wait for to get it unassigned. Because we don't need to waste others time.

The Patch looks good to me .

tameeshb’s picture

Owh, sorry about that, but why was the alert put there? I didn't quite get that.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Hm, I am not sure about the intention of that JS security test, but if we don't know the intention it seems to be a problem to remove it.

yoroy’s picture

Issue tags: +JavaScript
droplet’s picture

Issue tags: +Needs security review

I don't understand why it's filtering the JS. The Color preview markups is coded by developers. If it's intent to test XSS, it seems not enough, though.

manojapare’s picture

IMHO https://www.drupal.org/node/1167366#comment-11919727 we should not be removing the script tag because test is clearly checking for

/**
   * Ensures color preview.html is sanitized.
   */
  function testColorPreview() {
    // Install the color test theme.

So we should be sanitizing the html.
For that we can use Xss::filterAdmin().

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
8.9 KB
1023 bytes

@Manoj Thanks for the help

drupalmonkey’s picture

+      {% trans %} This is a block of example content. This content is here to demonstrate what actual content would look like in the color scheme you configure. <a>This is an example link</a>, is is here to demonstrate how background colors you configure may affect the readability of links. This content is here to demonstrate what actual content would look like in the color scheme you configure. {% trans %}

Second tag should be "endtrans".

rakesh.gectcr’s picture

Status: Needs review » Needs work
rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
8.91 KB
1.1 KB
tkoleary’s picture

Status: Needs review » Reviewed & tested by the community

Loos like all the trans tags are correct now.

droplet’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/color/color.module
@@ -300,8 +301,11 @@ function template_preprocess_color_scheme_form(&$variables) {
+  $unsanitized_preview = $twig->renderInline(file_get_contents($preview_html_path));
+  $sanitized_preview = Xss::filterAdmin($unsanitized_preview);
+  $variables['html_preview']['#markup'] = $sanitized_preview;

this is double-escaped if I'm correct.

#markup has auto-escape.
Xss::filterAdmin is filtered once. It returned string $string, not the MarkupInterface object.

When it passing to #markup, that will be 2nd escape.

And shouldn't it use this instead? https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

and it should Xss::filterAdmin before /InlineTemplate::preRenderInlineTemplate, right?

rakesh.gectcr’s picture

@droplet

Can you please check the https://www.drupal.org/pift-ci-job/591472. If you can provide more information about the same, Y it is getting failed rather than sanitizing, we can work on it.

See

--- a/core/themes/bartik/color/color.inc
+++ b/core/themes/bartik/color/color.inc
@@ -116,7 +116,7 @@
 
   // Preview files.
   'preview_library' => 'bartik/color.preview',
-  'preview_html' => 'color/preview.html',
+  'preview_html' => 'color/preview.html.twig',

We only give the path of the template. We are not given array contains the template element. So we are using $twig->renderInline(file_get_contents($preview_html_path)); instead of preRenderInlineTemplate .

droplet’s picture

@see patch. This has twig template cached also.

rakesh.gectcr’s picture

Status: Needs review » Reviewed & tested by the community

@droplet, Great, thanks for the patch. Looks like this good to go in. Making it to RTBC.

xjm’s picture

Title: Users do not understand "Lorem Ipsum" under Color Scheme preview » Replace "Lorem Ipsum" Color preview HTML templates with example text in Twig templates
xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots, +Needs manual testing, +Needs tests

Thanks everyone. This is looking much better!

Now that we've improved the patch, we should have another round of documented manual testing and add the following:

  1. Before-and-after screenshots of each improved preview template in the UI. (https://www.drupal.org/files/issues/After_apply_patch.png from @brahmjeet789 is a good example of an "after" screenshot from an earlier version of the patch. When we create screenshots, we should also have "Before" screenshots without the patch to compare, and embed them in the issue summary. See https://www.drupal.org/contributor-tasks/add-screenshots for more information on adding screenshots.)
  2. Manual testing and demonstration of the translatability (a screenshot will work well there too).
  3. Automated tests for the translatability. I went back and forth about whether to ask for automated tests for the translation aspect of the preview feature, but it is a new feature we're adding. We have to add the translatability feature in order to not regress multilingual while replacing the lorem ipsum (because lorem ipsum, as fake-Latin nonsense, is the same in every language, but real example content is not). Therefore, let's also add a small automated test for the translatability. We should check for existing tests of the preview functionality itself first.
  4. +++ b/core/modules/color/color.module
    @@ -15,6 +15,7 @@
    +use Drupal\Component\Utility\Xss;
    
       // Attempt to load preview HTML if the theme provides it.
    -  $preview_html_path = \Drupal::root() . '/' . (isset($info['preview_html']) ? drupal_get_path('theme', $theme) . '/' . $info['preview_html'] : drupal_get_path('module', 'color') . '/preview.html');
    -  $variables['html_preview']['#markup'] = file_get_contents($preview_html_path);
    +  $preview_html_path = isset($info['preview_html']) ? drupal_get_path('theme', $theme) . '/' . $info['preview_html'] : drupal_get_path('module', 'color') . '/preview.html.twig';
    +  $variables['html_preview']['#markup'] = \Drupal::service('twig')->loadTemplate($preview_html_path, NULL)->render(array());
    

    Is the use statement now unused?

cilefen’s picture

+++ b/core/modules/color/preview.html.twig
@@ -0,0 +1,11 @@
+      {% trans %} This is a block of example content. This content is here to demonstrate what actual content would look like in the color scheme you configure. <a>This is an example link</a>, is is here to demonstrate how background colors you configure may affect the readability of links. This content is here to demonstrate what actual content would look like in the color scheme you configure. {% endtrans %}

There is a repeated sentence here.

tameeshb’s picture

Avoiding repetition from #113 with interdiff from #109

tameeshb’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs screenshots, -Needs manual testing
FileSize
117.21 KB
94.86 KB

Before:

After(applying patch on #114 ):

wturrell’s picture

Replaced <a>This is.. with <a href="#">This is...
With former, mouse pointer doesn't change to a hand when hovering over link, which I think is confusing.

@xjm - #112 item 4 - yep, removed, not used anywhere.

(So what's outstanding is tests including translations.)

Status: Needs review » Needs work

The last submitted patch, 116: 1167366-116.patch, failed testing.

wturrell’s picture

Look like 1167366-114.patch included a couple of unrelated contact form files. Let's see if it passes now…

wturrell’s picture

Status: Needs work » Needs review

The last submitted patch, 114: 1167366-114.patch, failed testing.

aviseksingh’s picture

FileSize
100.89 KB
131.03 KB

NR #119
@wturrell The patch looks great. tested on 8.4.x-dev
Attaching screenshot before and after.

aviseksingh’s picture

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

Status: Reviewed & tested by the community » Needs work

Thanks. We still need tests for the translation.

Jeff Burnz’s picture

String review.

Just a general commit I don't understand why some lines wrap, others don't, and the ones that do wrap do so in no particular way. Can we impose the 80 char limit here and tidy up wrapping?

+++ b/core/modules/color/preview.html.twig
@@ -0,0 +1,11 @@
+      {% trans %} This is a block of example content. This content is here to demonstrate what actual content would look like in the color scheme you configure. <a href="#">This is an example link</a>, is is here to demonstrate how background colors you configure may affect the readability of links. This is some more text to demonstrate what actual content would look like in the color scheme you configure. {% endtrans %}

is is

+++ b/core/themes/bartik/color/preview.html.twig
@@ -0,0 +1,67 @@
+        <li><a>{% trans %} The first tab {% endtrans %}</a></li>
+        <li><a>{% trans %} Another tab {% endtrans %}</a></li>
+        <li><a>{% trans %} The last tab {% endtrans %}</a></li>

"Another" is not an ordinal number. Suggest something simple e.g. Tab One, Tab Two, Tab Three.

+++ b/core/themes/bartik/color/preview.html.twig
@@ -0,0 +1,67 @@
+              <li><a>{% trans %} One footer link {% endtrans %}</a></li>
+              <li><a>{% trans %} Another footer link {% endtrans %}</a></li>
+              <li><a>{% trans %} A third footer link {% endtrans %}</a></li>
+              <li><a>{% trans %} The Last footer link {% endtrans %}</a></li>

Ordinality not adhered to here either.

wturrell’s picture

FileSize
8.66 KB
5.19 KB

Revised patch with Jeff Burnz's changes. (Skipping a further test run at this stage to reduce noise.)

Jeff Burnz’s picture

More nit picking...

  1. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,73 @@
    +    <div class="color-preview-logo"><img
    +              src="../../../core/themes/bartik/logo.svg" alt="Site Logo"/></div>
    

    In this particular case we're not really bound to the 80 char limit, which is for text, not code. That said tags on a new line is easier to read:

    <div class="color-preview-logo">
      <img src="../../../core/themes/bartik/logo.svg" alt="Site Logo"/>
    </div>
    
  2. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,73 @@
    +      <h1 class="color-preview-page-title">{% trans %} This is an example
    +        heading {% endtrans %}</h1>
    

    Again new line reads better (code legibility) - perhaps we can impose that all headings do this, which is not part of any official style-guide, but then gain this is the only file like this in the entire system afaict.

    <h1 class="color-preview-page-title">
      {% trans %}This is an example heading{% endtrans %}
    </h1>
    
  3. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,73 @@
    +            {% trans %} This is a block of example content. This content is here
    +          to demonstrate what actual content would look like in the color
    +          scheme you configure. <a href="#">This is an example link</a>, to
    +          demonstrate how background colors you configure may affect the
    +          readability of links. Another sentence to demonstrate what actual
    +          content would look like in the color scheme you configure.
    +          <a href="#">This is another example link.</a> This is the end of this
    +          block of example content. {% endtrans %}
    

    Theres a couple of the blocks where the indentation does not align properly with the trans tag. The wrapping is good, just the indentation is still a bit off.

+++ b/core/modules/color/preview.html.twig
@@ -0,0 +1,16 @@
+      {% trans %} Example heading {% endtrans %}

White space controllers in tags etc - this has been done all over the place in this file, I looked up our Twig styleguide and it says we should remove these whites spaces: https://www.drupal.org/docs/develop/coding-standards/twig-coding-standards

If you can remove the whitespace legibly, do so

swarad07’s picture

Status: Needs work » Needs review
FileSize
9.7 KB
2.81 KB

Made changes as per feedback in #126

Jeff Burnz’s picture

Status: Needs review » Needs work

Great stuff @swarad07, looking much better, just a couple more nit picks (yes sorry I am that guy :)

  1. +++ b/core/modules/color/preview.html.twig
    @@ -0,0 +1,16 @@
    +        {% trans %} This is a block of example content. This content is here to
    

    Indent incorrect.

  2. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,75 @@
    +    <div class="color-preview-logo"><img
    +              src="../../../core/themes/bartik/logo.svg" alt="Site Logo"/>
    +    </div>
    

    img tag on new line

Jeff Burnz’s picture

+++ b/core/modules/color/preview.html.twig
@@ -0,0 +1,16 @@
+      {% trans %} Example heading {% endtrans %}

I still tend to think we should remove all white space after and before trans tags, or use newlines (like the original CR shows), but I suppose it depends on how this tag works, and if the actual translation system is work fine with it etc.

Deferring to those who know better.

Gábor Hojtsy’s picture

Agreed, let's remove the whitespaces at start/end of trans.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
9.69 KB

Applying changes from #128, #129 and ##13
Should the spaces before/after the trans tags be removed in core/themes/bartik/color/preview.html.twig too?

tameeshb’s picture

Jeff Burnz’s picture

Status: Needs review » Needs work
  1. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,75 @@
    +     <h1 class="color-preview-page-title">{% trans %}
    +        This is an example heading{% endtrans %}
    +      </h1>
    

    This one I think like this:

    <h1 class="color-preview-page-title">
      {% trans %}This is an example heading{% endtrans %}
    </h1>
    
  2. +  <div class="color-preview-footer-wrapper">
    +    <div class="color-preview-footer-columns clearfix">
    +      <div class="preview-footer-column">
    +        <div class="preview-block">
    +          <h2>{% trans %}Example heading{% endtrans %}</h2>
    +          <div class="content">
    +            {% trans %}This is a block of example content. This content is here
    +            to demonstrate what actual content would look like
    +            in the color scheme you configure.{% endtrans %}
    +          </div>
    

    I can see some lines not wrapping quite right in here, the paste is not really showing it, see the patch.

This is looking great - really good stuff, feeling relaxed again already :)

jofitz’s picture

Status: Needs work » Needs review
FileSize
9.66 KB
1.08 KB

Corrections from code review by @Jeff Burnz.

Jeff Burnz’s picture

#134 looks good.
#112 to go.

jofitz’s picture

Status: Needs review » Needs work
Bojhan’s picture

Awesome, I am looking forward to seeing the latin gone :)

rakesh.gectcr’s picture

@Jo Fitzgerald, I think patch [#134] having unwanted changes.

+</div>
diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
index 25d498e..30deff4 100644
--- a/sites/default/default.settings.php
+++ b/sites/default/default.settings.php
@@ -144,11 +144,6 @@
  * @code
  *   'prefix' => 'main_',
  * @endcode
- *
- * Per-table prefixes are deprecated as of Drupal 8.2, and will be removed in
- * Drupal 9.0. After that, only a single prefix for all tables will be
- * supported.
- *
  * To provide prefixes for specific tables, set 'prefix' as an array.
  * The array's keys are the table names and the values are the prefixes.
  * The 'default' element is mandatory and holds the prefix for any tables
@@ -270,11 +265,6 @@
  * by the user.
  *
  * @see install_select_profile()
- *
- * @deprecated in Drupal 8.3.0 and will be removed before Drupal 9.0.0. The
- *   install profile is written to the core.extension configuration. If a
- *   service requires the install profile use the 'install_profile' container
- *   parameter. Functional code can use \Drupal::installProfile().
  */
 # $settings['install_profile'] = '';
rakesh.gectcr’s picture

Please check the patch and inter diff files

rakesh.gectcr’s picture

Status: Needs work » Needs review
wturrell’s picture

I've split the translations up into smaller fragments. I wasn't able to find any specific best practice on translatable strings, so I'm working on the assumption the smaller they are the more we can reuse them, the more likely translators are to attempt to translate them, and, where multiple translations are submitted, the shorter the phrase, the greater chance of matches.

droplet’s picture

Issue tags: -JavaScript

Split the translation is bad. That's paragraph and we should not force translate sorting in that ordering.

For example:

This is a block of example content. This content is here to demonstrate what actual content would
look like in the color scheme you configure.

In other languages, it may translate into

This content is here to demonstrate what actual content would
look like in the color scheme you configure. This is a block of example content.

Or

This content is here to demonstrate. This is a block of example content. What actual content would
look like in the color scheme you configure.

I think it can pass via TRIM filter if we want to tidy it up a little more.

** I removed the JavaScript tag. It hasn't involved with the JS directly, or even not indirectly. Thanks All.

rakesh.gectcr’s picture

Assigned: Unassigned » rakesh.gectcr
Status: Needs review » Needs work
rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
10.23 KB
10.23 KB
2.92 KB

Done the suggestions on the previous comment.

Trying to Write the test, Before that , Trying to run the 'ColorSafePreviewTest' and Its ran me into the issue

Testing started at 12:38 AM ...
#!/usr/bin/env php
PHPUnit 4.8.34 by Sebastian Bergmann and contributors.

E

Time: 12.09 seconds, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\color\Functional\ColorSafePreviewTest::testColorPreview
Exception: Warning: mkdir(): Permission denied
Drupal\Component\PhpStorage\FileStorage->createDirectory()() (Line: 157)

Any one has any clue about this please let me know

Status: Needs review » Needs work

The last submitted patch, 144: 1167366-144.patch, failed testing.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
273.28 KB
429.43 KB
14.26 KB
4.52 KB

I applied the suggestion on [#142] and tested manually and added a small automated test for the translation, which will ensure the text is wrapped inside {% trans %} tags in the preview file. Please find the below screenshots for the same.

In English

In Malayalam

Please check screen recording for the same https://youtu.be/exsJIRLmt1o

Status: Needs review » Needs work

The last submitted patch, 146: 1167366-146.patch, failed testing.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
14.27 KB
691 bytes

Well, there was a typo on test.

rakesh.gectcr’s picture

Assigned: rakesh.gectcr » Unassigned
mikeohara’s picture

Issue tags: +Baltimore2017

I am going to visually test this as part of DrupalCon Baltimore 2017

mikeohara’s picture

Tested this and it seems to be behaving as expected in 8.4.x

mikeohara’s picture

Status: Needs review » Reviewed & tested by the community
star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Thanks everyone for the work on this so far :)

  1. +++ b/core/modules/color/preview.html.twig
    @@ -0,0 +1,17 @@
    +    <h2>
    +      {% trans %}Example heading{% endtrans %}
    +    </h2>
    +    <p>
    +      {% trans %}This is a block of example content.{% endtrans %}
    +      {% trans %}This content is here to demonstrate what actual content would
    +      look like in the color scheme you configure.{% endtrans %}
    +      {% trans %}<a href="#">This is an example link</a>, to demonstrate how
    +      background colors you configure may affect the readability of links.{% endtrans %}
    +      {% trans %}This is some more text to demonstrate what actual content would
    +      look like in the color scheme you configure.{% endtrans %}
    +    </p>
    

    I'm a bit rusty on our conventions here but this seems like a lot of translatable strings when it could be just one or two. Similar cases in other parts of the patch too.

  2. +++ b/core/modules/color/tests/src/Functional/ColorSafePreviewTest.php
    @@ -44,10 +44,11 @@ public function testColorPreview() {
    +    // Markup is being printed from a TWIG file located in:
    

    Minor: Twig is not acronym so this should be "Twig" instead of "TWIG".

  3. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,62 @@
    +class ColorTranslationPreviewTest extends BrowserTestBase
    +{
    

    Coding standards: The bracket should be on the same line as the class declaration.

    https://www.drupal.org/docs/develop/coding-standards/object-oriented-cod...

  4. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,62 @@
    +  protected function setUp()
    +  {
    ...
    +  public function testTranslationPreview()
    +  {
    

    Similarly here.

  5. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,62 @@
    +    $this->rebuildContainer();
    +
    +  }
    

    Minor: Extra blank line at the bottom of the setUp method.

  6. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,62 @@
    +
    +    // Markup is being printed from a TWIG file located in:
    +    // core/modules/color/tests/modules/color_test/themes/color_test_theme/color/preview.html.twig
    +    $url_preview = 'core/modules/color/tests/modules/color_test/themes/color_test_theme/color/preview.html.twig';
    +    $this->preview = file_get_contents($url_preview);
    +    $this->assertRegExp('/{% trans %}/', $this->preview);
    +    $this->assertRegExp('/{% endtrans %}/', $this->preview);
    +
    +  }
    +
    +}
    

    Minor: Extra blank lines at the start and end of the testTranslationPreview method.

    Also missing newline at end of this file per https://www.drupal.org/docs/develop/standards/coding-standards#indenting.

leolandotan’s picture

I'll work on the feedback from @Cottser.

leolandotan’s picture

Here I have done the feedback items from @Cottser 's latest comment.

This change also includes some additional changes from previous feedbacks:

Hope everything is in order.

Thanks!

Status: Needs review » Needs work

The last submitted patch, 155: replace_color_module_template_lipsum_text-1167366-155.patch, failed testing.

c.nish2k3’s picture

Status: Needs work » Needs review
FileSize
14.08 KB

Fixing the error

leolandotan’s picture

Here is in interdiff of 155 to 156.

Noticed the following in 156:

  • The text wrappings were reverted back to the ones not fitting the 80 character rule(recommendation reference in #155),
  • Th "new line headings" were reverted back to single lines(i think this caused the previous error from the automated test recommendation reference in #155)
  • Some indentation issues in core/modules/color/preview.html.twig
  • A blank line added in core/modules/color/tests/src/Functional/ColorSafePreviewTest.php
  • Reverted the docblock comment in core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php from using a third person singular present tense verb and this exceeds the 80 character line limit.

I'm not sure how to proceed yet with this change If what is recommended. I'm still looking at why the tests fail when we use new lines for headings tags and its content i.e.:

<h2>
  {% trans %}Example heading{% endtrans %}
</h2>  
rakesh.gectcr’s picture

Status: Needs review » Needs work

The test has to be rewritten, Currently, the test checking, the text is wrapped with {% trans %} {% endtrans %}. tag or not. Instead of that test should check. Whether the text is given in the tag is getting translated or not?

Gábor Hojtsy’s picture

Agreed with @rakesh.gectcr from #159:

+++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
@@ -0,0 +1,55 @@
+  /**
+   * Ensuring the the text in the preview.html.twig file is wrapped with trans tag.
+   */
+  public function testTranslationPreview() {
+    // Markup is being printed from a Twig file located in:
+    // core/modules/color/tests/modules/color_test/themes/color_test_theme/color/preview.html.twig
+    $url_preview = 'core/modules/color/tests/modules/color_test/themes/color_test_theme/color/preview.html.twig';
+    $this->preview = file_get_contents($url_preview);
+    $this->assertRegExp('/{% trans %}/', $this->preview);
+    $this->assertRegExp('/{% endtrans %}/', $this->preview);
+  }

Yeah this does not really ensure anything except that at least one opening and one closing trans tag exists in the file.

wturrell’s picture

@Gábor - what's the best practice for breaking up lengthy translatable strings into chunks? See my changes in #141 and droplet's comments in #142 (also @cottser #153): should we split into small chunks to maximise reuse, or do whole paragraphs need to be kept intact for rearranging of sentences in certain languages? If latter, then we need to revert my changes...

Gábor Hojtsy’s picture

There are quite some inconsistencies in the patch as to how it treats long paragraphs. I would say we did not encounter long text in twig templates before, but we can apply similar rules as there are for hook_help() snippets, you can look there in terms of ways to breaking up text. For list items for example, we would translate them one by one, even if they are part of a bigger paragraph.

star-szr’s picture

Thank you @Gábor Hojtsy for that info and sorry if my comment caused any confusion/unnecessary churn.

c.nish2k3’s picture

Instead of that test should check. Whether the text is given in the tag is getting translated or not?

What is the best way to check this? The output is going to be in a language whihc we do not know now.

droplet’s picture

yoroy’s picture

Priority: Minor » Normal
neerajpandey’s picture

Issue tags: -UMN 2011, -Baltimore2017
neerajpandey’s picture

Assigned: Unassigned » neerajpandey
TR’s picture

Issue tags: +UMN 2011

I restored the UMN 2011 tag. This tag is used to identify issues, including this one, which were identified by the University of Minnesota usability study in 2011.

rakesh.gectcr’s picture

rakesh.gectcr’s picture

Hi I am working on the test, the following is the code, I got struck on translation the following is the initial code.

<?php

namespace Drupal\Tests\color\Functional;

use Drupal\Tests\BrowserTestBase;

/**
 * Tests will check the text in the loaded preview file is translatable.
 *
 * @group color
 */
class ColorTranslationPreviewTest extends BrowserTestBase {

  /**
   * Modules to enable.
   *
   * @var array
   */
  public static $modules = ['color', 'color_test', 'language',];

  /**
   * Profile to enable.
   *
   * @var string
   */
  protected $profile = 'standard';

  /**
   * Ensuring the the text in the preview.html.twig file is wrapped with trans tag.
   */
  public function testTranslationPreview() {
    $account = $this->drupalCreateUser(['administer themes']);
    $this->drupalLogin($account);
    $this->drupalGet('admin/appearance/settings/bartik', array(), array(
      'Accept-Language' => 'fr',
    ));
    $this->assertSession()->statusCodeEquals(200);
    $this->assertSession()->pageTextContains('This is an example heading');

  }
}

Need Guidance on

  1. How to set up and test translation?
rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
14.36 KB
2.79 KB

Looks like I figured it out. :) Kindly review it.

dawehner’s picture

Thank you for working on the test!

  1. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,65 @@
    +  /**
    +   * Profile to enable.
    +   *
    +   * @var string
    +   */
    +  protected $profile = 'standard';
    

    Here is a question: why do we need to install standard module? Usually we try to avoid installing standard to a) limit the time b) avoid sideffects

  2. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,65 @@
    +    // Add French language.
    +    $edit = [
    +      'predefined_langcode' => 'fr',
    +    ];
    +    $this->drupalPostForm('admin/config/regional/language/add', $edit, t('Add language'));
    +    $this->rebuildContainer();
    

    I'd have put creating a language into the setup process. Any reason you have chosen to not do that? All you need is basically \Drupal\language\Entity\ConfigurableLanguage::createFromLangcode('fr')->save()

rakesh.gectcr’s picture

Status: Needs review » Needs work
rakesh.gectcr’s picture

Assigned: neerajpandey » Unassigned
Status: Needs work » Needs review
FileSize
14.22 KB
1.33 KB

Thanks @dawehner for the review.

  1. about protected $profile = 'standard';
  2. We need to have Bartik theme installed for testing

    //Checking the string 'Example heading' is translating to French.
        $this->drupalGet('fr/admin/appearance/settings/bartik');
  3. Thanks for that, wa not aware of it, In fact I found out from Drupal\Tests\language\Functional\LanguageLocaleListTest;
rakesh.gectcr’s picture

Status: Needs review » Needs work

Talked to @dawehner and @phenaproxima

phenaproxima [7:43 PM] 
@rakeshjames \Drupal::service('theme_installer')->install(['mytheme'])
[7:43] 
@rakeshjames That oughta do it...but then there is the question of making that theme the default. Which I'm less sure about how to do.

need to Change code accordingly.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
14.25 KB

Mentioned all the above comments,

Please review...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs security review, -Needs tests

This looks totally reasonable to me now. It has tests now.

Droplet in #100 required a security review. The current approach now though seems to be to use twig template, which are safe of XSS issues. Given that I removed the tag.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,74 @@
    +    <div class="color-preview-site-name">{% trans %}Bartik{% endtrans %}</div>
    

    The name of the theme should not be translatable. This could be super confusing for a translator.

  2. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,74 @@
    +          {% trans %}This content is here to demonstrate what actual content would
    +          look like in the color scheme you configure. This is a block of example content.{% endtrans %}
    ...
    +            {% trans %}This content is here to demonstrate what actual content would
    +            look like in the color scheme you configure. This is a block of example content.{% endtrans %}
    

    We should make the order of the sentences consistent with color module.

  3. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,74 @@
    +          {% trans %}This content is here to demonstrate what actual content would
    ...
    +          {% trans %}<a href="#">This is an example link</a>, to
    +          demonstrate how background colors you configure may affect the
    +          readability of links.{% endtrans %}
    +          {% trans %}Another sentence to demonstrate what actual content would
    +          look like in the color scheme you configure.{% endtrans %}
    +          {% trans %}<a href="#">This is another example link.</a>{% endtrans %}
    +          {% trans %}This is the end of this block of example content.{% endtrans %}
    

    This should also be just a single trans block.

  4. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,74 @@
    +      <img src="../../../core/themes/bartik/logo.svg" alt="Site Logo"/>
    

    The alt text should be translatable

Ada Hernandez’s picture

#1 done, #2 and #3 I made like color module with h2 and p, #4 done

Ada Hernandez’s picture

Status: Needs work » Needs review
vegantriathlete’s picture

Issue tags: +dcco2017

You can see about getting this to RTBC and / or creating an issue summary.

greggmarshall’s picture

#DCCO2017 working on this

greggmarshall’s picture

Reviewed the code and it resolves all issues in #179

Spun up test site, added translations to French for most of the content using Google Translate to generate, content appears translated. Screenshots attached.

greggmarshall’s picture

Status: Needs review » Reviewed & tested by the community
rakesh.gectcr’s picture

@greggmarshall Thank you for making it o RTBC. +1

lauriii’s picture

Component: Bartik theme » color.module
Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color/preview.html.twig
    @@ -0,0 +1,8 @@
    +    <p>{% trans %}Sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud <a href="#">exercitation ullamco</a> laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.{% endtrans %}</p>
    
    +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,75 @@
    +      <img src="../../../core/themes/bartik/logo.svg" alt="{% trans %}Site Logo{% endtrans %}"/>
    ...
    +          {% trans %}This content is here to demonstrate what actual content would
    +          look like in the color scheme you configure. This is a block of example content.{% endtrans %}
    ...
    +            {% trans %} This is a block of example content. This content is here to demonstrate what actual content would
    ...
    +            <a href="#">This is an example link</a>, to demonstrate how background colors you configure may affect the
    ...
    +            {% trans %}This content is here to demonstrate what actual content would
    +            look like in the color scheme you configure. This is a block of example content.{% endtrans %}
    

    These should be formatted so that the line length doesn't exceed 80 characters.

  2. There's a few other coding standard failures that are visible in the CI.
harsha012’s picture

fixed as per #187

harsha012’s picture

Status: Needs work » Needs review
vegantriathlete’s picture

Issue tags: -dcco2017

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rakesh.gectcr’s picture

Status: Needs review » Reviewed & tested by the community

Looks like @laurriii's comment has been applied. Moving back to RTBC

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,78 @@
    +      {% endtrans %}"/>
    

    Could we use the short syntax for this instead so we don't have to split this to 2 lines?

    {{ 'Site Logo'|t }}
    
  2. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,78 @@
    +          {% trans %}This content is here to demonstrate what actual content would
    +          look like in the color scheme you configure. This is a block of example
    ...
    +            demonstrate what actual content would look like in the color scheme you
    +            configure.<a href="#">This is an example link</a>, to demonstrate how
    ...
    +            Another sentence to demonstrate what actual content would look like in
    

    There's still lines that exceed the maximum line length

  3. +++ b/core/themes/bartik/color/preview.html.twig
    @@ -0,0 +1,78 @@
    +            {% trans %} This is a block of example content. This content is here to
    

    There's unneeded space before the first word.

rakesh.gectcr’s picture

Assigned: Unassigned » rakesh.gectcr

Working on this

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
14.29 KB
1.95 KB

@lauriii

  1. After applying the short syntax also its comes more than 80. So I set a variable for the log's path and applied.
  2. Done
  3. Done
rakesh.gectcr’s picture

jofitz’s picture

Status: Needs review » Needs work
+++ b/core/themes/bartik/color/preview.html.twig
@@ -0,0 +1,78 @@
+          {% trans %}This content is here to demonstrate what actual content would
+          look like in the color scheme you configure. This is a block of example
+          content.{% endtrans %}
...
+            {% trans %}This is a block of example content. This content
+            is here to demonstrate what actual content would look like in the
+            color scheme you configure.<a href="#">This is an example link</a>,
+            to demonstrate how background colors you configure may affect the
+            readability of links. Another sentence to demonstrate what actual
+            content would look like in the color scheme you configure.
+            <a href="#">This is another example link.</a>This is the
+            end of this block of example content.{% endtrans %}

There are still a couple of sections that don't fill the 80 characters correctly.

Ada Hernandez’s picture

Status: Needs work » Needs review
FileSize
14.22 KB
844 bytes

Jo Fitzgerald -> 80 characters set correctly

rakesh.gectcr’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, Moving to RTBC

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color/color.inc
    @@ -29,5 +29,5 @@
    +  'preview_html' => 'color/preview.html.twig',
    
    +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color/preview.html.twig
    @@ -0,0 +1,8 @@
    +<div class="color-preview">
    +  <div id="text">
    +    <h2>{% trans %}TEST COLOR PREVIEW{% endtrans %}</h2>
    +    <p>{% trans %}Sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud <a href="#">exercitation ullamco</a> laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.{% endtrans %}</p>
    +  </div>
    +  <div id="img"></div>
    +</div>
    +<script>alert("security filter test");</script>
    

    Are we certain that moving this to twig doesn't inadvertently output the script tag? Can someone enable that theme and test manually?
    We have a test at \Drupal\Tests\color\Functional\ColorSafePreviewTest (which I note still reference the .html files, so needs to be updated here) but I'd like the extra security of a manual test if possible.

  2. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,60 @@
    +}
    \ No newline at end of file
    

    nit missing newline (can be fixed on commit)

A further question - it feels like we're doing two things here - moving to twig *and* changing the example text, which feels like it violates our scope policy.

joelpittet’s picture

Assigned: rakesh.gectcr » Unassigned
Status: Needs review » Needs work

I agree, the copy changes need to move to a separate issue. Regarding the script tag concern in #200.1, I'm quite confident because it's stored on the same #markup and there is a test to ensure noraw.

Ivan Berezhnov’s picture

Issue tags: +CSKyiv18
MaskyS’s picture

Issue tags: -UMN 2011

Removed an old tag :)

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mohit1604’s picture

Adding test in patch #198 for version 8.6.x :)

joelpittet’s picture

So the tasks:

  1. Keep the copy/text changes only in this issue and update the issue summary
  2. Make a follow-up issue for the conversion in twig
  3. Manually test both issues
benjifisher’s picture

Issue tags: +Nashville2018
jphelan’s picture

Review patch #198 and it works for me but was missing a couple spaces in the test. I also update the test file text to match the color twig file. The templates have already been converted to twig as well so a separate issue is not necessary.

jphelan’s picture

Status: Needs work » Needs review
benjifisher’s picture

@jphelan Thanks for working on this!

In addition to the patch, can you upload an interdiff so that we can easily see what has changed since the previous patch? There are instructions here: https://www.drupal.org/documentation/git/interdiff.

jphelan’s picture

Sorry, the last patch was missing some of the changes. This is the correct patch.

jphelan’s picture

FileSize
2.66 KB

Yes, here is the interdiff.

Status: Needs review » Needs work

The last submitted patch, 211: color-replace-lorem-ipsum-example-text-1167366-209-D8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jphelan’s picture

Reroll patch for failed test.

Status: Needs review » Needs work
rakesh.gectcr’s picture

Issue tags: +Nwdug_may18
tresti88’s picture

Assigned: Unassigned » tresti88

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

pradeepjha’s picture

Re-rolling #198 patch for 9.1.x

pradeepjha’s picture

Assigned: tresti88 » Unassigned
Status: Needs work » Needs review

It's in idle state from long. I'm moving it in review.

pradeepjha’s picture

Status: Needs review » Needs work
narendra.rajwar27’s picture

Assigned: Unassigned » narendra.rajwar27
narendra.rajwar27’s picture

Status: Needs work » Needs review
FileSize
715 bytes
14.41 KB

Re-rolled the patch for 9.1.x.

Status: Needs review » Needs work

The last submitted patch, 227: 1167366-227.patch, failed testing. View results

narendra.rajwar27’s picture

Status: Needs work » Needs review
FileSize
899 bytes
14.64 KB

Status: Needs review » Needs work

The last submitted patch, 229: 1167366-229.patch, failed testing. View results

narendra.rajwar27’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
14.48 KB

Fix for failed test cases.

narendra.rajwar27’s picture

Assigned: narendra.rajwar27 » Unassigned

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Asha Nair’s picture

Patch #231 failed to apply

benjifisher’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

I confirmed that the patch in #231 does not apply to the current 9.4.x.

I am setting the issue status to NW and adding the tag for a reroll.

ravi.shankar’s picture

Added reroll of patch #231.

andregp’s picture

Status: Needs work » Needs review
FileSize
14.54 KB
1.6 KB

Fixed Coding Standards on patch #238 (Custom Commands Failed test)

Edit: all right, my patch failed too :/

andregp’s picture

Assigned: Unassigned » andregp
Status: Needs review » Needs work

Sending back to Needs Work as it failed testing.

I'll work on this.

andregp’s picture

Assigned: andregp » Unassigned
Status: Needs work » Needs review
FileSize
14.61 KB
1.58 KB
1.33 KB

Okay, I opened the tests to see the result log (which I should have done before) and noticed the patches #238 and #239 are actually failing due to a "misspelled" word (Exemple) which is in French so I just added a @codingStandardsIgnoreLine before these lines. I hope the patch will pass on the tests now.

TR’s picture

Using @codingStandardsIgnoreLine is not the way to ignore spelling problems.
See the change record at https://www.drupal.org/node/3122084 for how to fix this.

ranjith_kumar_k_u’s picture

Status: Needs review » Needs work

The last submitted patch, 243: 1167366-243.patch, failed testing. View results

andregp’s picture

Thank you @TR for the tip and thanks @ranjith_kumar_k_u for fixing it!
New patch (as #243 got some failed tests).

andregp’s picture

Status: Needs work » Needs review

I forgot to change status.

Status: Needs review » Needs work

The last submitted patch, 245: 1167366-245.patch, failed testing. View results

ravi.shankar’s picture

andregp’s picture

@ravi.shankar the test errors on #245 were due to certain strings not being found on the page (the "Example heading" and "Exemple de titre" strings as you can see on the error logs).

I'm not sure how making $modules protected at static $modules = ['color', 'color_test', 'language', 'locale']; will solve these errors. Could you explain why this change on the code?

Okay, you probably did it to solve a deprecation warning (according to https://www.drupal.org/node/2909426)

TR’s picture

You should also be using {@inheritdoc} for the $modules property docblock to bring this new test up to current standards.

+ protected $defaultTheme = 'stark';
Why define 'stark' here, then override it and install 'bartik' later? Why not just use 'bartik' here?

+    // Add a default locale storage.
+    $this->storage = ...

You're assigning values to $this->storage, but you didn't actually define a $storage property on the test class. PHP, unlike most programming languages, won't complain about this, but it's bad practice. Explicitly declaring the property adds documentation and declares intent, which helps to avoid errors in the future.

andregp’s picture

So, after a long time analyzing many previous patches I believe I got what was wrong and made a new patch with the all needed previous fixes.

Patch #231 passed all the tests, but then it needed a reroll.
Patch #231 changed $this->assertRaw('<h2>TEST COLOR PREVIEW</h2>'); to $this->assertNoRaw('<h2>Example heading</h2>'); at ColorSafePreviewTest.php and the reroll #238 used responseContains('<h2>Example heading</h2>') instead of responseNotContains('<h2>Example heading</h2>') (which is the correct replace for assertNoRaw). The reroll (#238) also messed a bit with the $preview_html_path at color.module

After remaking the reroll, I then added the changes from patch #239 (CS fixes), #243 (CS Fixes), and #248 (Deprecation fix)

Edit: Sorry, @TR I had this issue page open for a long time, so when I posted the patch I didn't see your comment thus I didn't work on your suggested changes. But I'll work on a new patch.

andregp’s picture

@TR
Regarding you question on #250.2

+ protected $defaultTheme = 'stark';
Why define 'stark' here, then override it and install 'bartik' later? Why not just use 'bartik' here?

I don't know why. It was like this when I worked on a new patch, maybe other developer who worked on that earlier may know.

andregp’s picture

Status: Needs work » Needs review
FileSize
14.94 KB
817 bytes

Some more fixes based on comment #250 (thanks @TR)

Johnny Santos’s picture

Assigned: Unassigned » Johnny Santos

I'm going to apply the patch and see if its working as planned to be.

Johnny Santos’s picture

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

applied the patch presented on comment #253.
its working as intended for the preview on bartik theme.

(sent the same picture twice, sorry)

Johnny Santos’s picture

Assigned: Johnny Santos » Unassigned
Status: Reviewed & tested by the community » Needs review

+1 RTBC

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Shiv Yadav’s picture

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

I have tasted on drupal 9.5.x-dev version and patch is applying properly
Patch #253 is working properly

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this issue! Just as a note, the Color module is being deprecated and remove from core, so this issue may become wontfix in the next month or so (or at least moved to the contrib queue, making a patch against core not work).

Meanwhile, some issues with the patch -- mostly nits, but the Bartik test dependency is something we definitely should not add.

  1. +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color/preview.html.twig
    @@ -0,0 +1,17 @@
    +    <p>
    +        {% trans %}This is a block of example content. This content is here to
    +          demonstrate what actual content would look like in the color scheme you
    +          configure. <a href="#">This is an example link</a>, to demonstrate how
    +          background colors you configure may affect the readability of links. This
    +          is some more text to demonstrate what actual content would look like in
    +          the color scheme you configure.{% endtrans %}
    

    This has different indentation from preview.html.twig and is being wrapped at about 83 characters instead of 80.

  2. +++ b/core/modules/color/tests/src/Functional/ColorSafePreviewTest.php
    @@ -49,14 +49,14 @@ public function testColorPreview() {
    +    // Markup is being printed from an Twig file located in:
    

    a Twig file, not "an".

  3. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,76 @@
    + * Tests will check the text in the loaded preview file is translatable.
    

    This isn't quite a sentence. Maybe:

    Tests that the color preview text is translatable.

  4. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,76 @@
    +    // Add French language.
    

    Nit: This should say either "Add French" or "Add the French language".

  5. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,76 @@
    +    // Installing bartik theme.
    

    This comment is unnecessary; the line of code speaks for itself. (If we were to have a comment, it should be "Install the Bartik theme.")

  6. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,76 @@
    +    \Drupal::service('theme_installer')->install(['bartik']);
    ...
    +    $this->drupalGet('fr/admin/appearance/settings/bartik');
    

    This won't work when both Color and Bartik are moved to contrib. Can we use the test theme instead? We shouldn't be adding more coupling to Bartik.

  7. +++ b/core/modules/color/tests/src/Functional/ColorTranslationPreviewTest.php
    @@ -0,0 +1,76 @@
    +    // Checking the string 'Example heading' is translating to French.
    

    This should say "translated to French" (not "translating").

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
14.91 KB
3.85 KB

Made changes as per comment #259, please review.

Status: Needs review » Needs work

The last submitted patch, 260: 1167366-260.patch, failed testing. View results

quietone’s picture

Project: Drupal core » Color backport
Version: 9.5.x-dev » 2.x-dev
Component: color.module » Code

Color has been removed from core, #3270899: Remove Color module from core.

ravi.shankar’s picture

Added patch #260 on Color module.