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.

Files: 
CommentFileSizeAuthor
#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 KBleolando.tan
#157 replace_color_module_template_lipsum_text-1167366-156.patch14.08 KBc.nish2k3
#155 interdiff-1167366-148-155.txt9.12 KBleolando.tan
#155 replace_color_module_template_lipsum_text-1167366-155.patch14.17 KBleolando.tan
#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
#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 KBJo Fitzgerald
#134 1167366-134.patch9.66 KBJo Fitzgerald
#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
#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

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 :)

Jo Fitzgerald’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.

Jo Fitzgerald’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
Cottser’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.

leolando.tan’s picture

I'll work on the feedback from @Cottser.

leolando.tan’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

leolando.tan’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.

Cottser’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.