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.
Comment | File | Size | Author |
---|---|---|---|
#263 | reroll_diff_260-263.txt | 11.56 KB | ravi.shankar |
#263 | 1167366-263.patch | 8.17 KB | ravi.shankar |
#260 | interdiff_253-260.txt | 3.85 KB | ravi.shankar |
#260 | 1167366-260.patch | 14.91 KB | ravi.shankar |
#258 | Screenshot from 2022-06-09 19-05-12.png | 80.57 KB | Shiv Yadav |
Comments
Comment #1
yoroy CreditAttribution: yoroy commentedHas to be fixed in head first. I think this is a Bartik specific feature.
Comment #2
JohnAlbinIts 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.
Comment #3
yoroy CreditAttribution: yoroy commentedLets 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.
Comment #4
Jeff Burnz CreditAttribution: Jeff Burnz commentedThere'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.
Comment #5
dcmistry CreditAttribution: dcmistry commented+1 to yoroy
Comment #6
joachim CreditAttribution: joachim commented-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.
Comment #7
droplet CreditAttribution: droplet commented-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
Comment #8
Jeff Burnz CreditAttribution: Jeff Burnz commented> 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.
Comment #9
longwaveAnother -1 to changing this. Lorem ipsum is the standard for dummy text. If this is to remain open, it should be minor at best.
Comment #10
Bojhan CreditAttribution: Bojhan commentedSorry, 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).
Comment #11
TR CreditAttribution: TR commentedPerhaps 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.
Comment #12
Jeff Burnz CreditAttribution: Jeff Burnz commentedWe 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.
Comment #13
joachim CreditAttribution: joachim commented> 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.
Comment #14
dcmistry CreditAttribution: dcmistry commentedI 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.
Comment #15
mrfelton CreditAttribution: mrfelton commentedI 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.
Comment #16
Bojhan CreditAttribution: Bojhan commentedComment #17
couturier CreditAttribution: couturier commentedWhy not compose several paragraphs using international words that are similar among many languages? For example:
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.
Comment #18
dcmistry CreditAttribution: dcmistry commented@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.
Comment #19
couturier CreditAttribution: couturier commentedThe 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.
Comment #20
emma.mariaPostponing 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.
Comment #21
couturier CreditAttribution: couturier commentedemma.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.
Comment #22
emma.mariaHi @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.
Comment #23
Jeff Burnz CreditAttribution: Jeff Burnz commented"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?
Comment #24
LewisNyman CreditAttribution: LewisNyman commentedhm, it's basically a flat html file in an iframe isn't it?
Comment #25
couturier CreditAttribution: couturier commentedComment #26
Jeff Burnz CreditAttribution: Jeff Burnz commentedTheres 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.
Comment #27
emma.mariaComment #28
SteffenRI 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
Comment #31
tkoleary CreditAttribution: tkoleary commentedComment #32
tameeshb CreditAttribution: tameeshb as a volunteer commented@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?
Comment #33
tkoleary CreditAttribution: tkoleary commented@tameeshb Certainly!
Comment #34
Bojhan CreditAttribution: Bojhan as a volunteer commentedIs this part of the theme?
Comment #35
tameeshb CreditAttribution: tameeshb as a volunteer commentedWith 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
Please suggest corrections in changes.
Comment #36
tameeshb CreditAttribution: tameeshb as a volunteer commentedPlease Review and suggest changes.
Comment #37
tameeshb CreditAttribution: tameeshb as a volunteer commented@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.
Comment #38
tameeshb CreditAttribution: tameeshb as a volunteer commentedComment #39
tkoleary CreditAttribution: tkoleary commented@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
Comment #40
yoroy CreditAttribution: yoroy at Roy Scholten commentedFrom #23:
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.
Comment #41
tkoleary CreditAttribution: tkoleary commentedShould be a twig file, then wrap the strings in %trans, I think.
Comment #42
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@tkoleary Yes I think this is a better idea.
Comment #43
brahmjeet789 CreditAttribution: brahmjeet789 as a volunteer and at gai Technologies Pvt Ltd commented@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
Comment #44
brahmjeet789 CreditAttribution: brahmjeet789 as a volunteer and at gai Technologies Pvt Ltd commentedComment #45
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedAlright then @brahmjeet789 @tkoleary What's to be done next?
Comment #46
brahmjeet789 CreditAttribution: brahmjeet789 as a volunteer and at gai Technologies Pvt Ltd commentedoops forgot to attach screenshot.
Comment #48
cilefen CreditAttribution: cilefen as a volunteer commentedWhat happened to the translatability?
Comment #49
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedWhy does this now say that it failed the testing when before it had passed the tests?
Build version is still same I suppose.
Comment #50
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedIt's passed again
Comment #51
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #52
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedSwitched back to RTBC after switch to "Needs work" due to failing a previous test.
Comment #53
cilefen CreditAttribution: cilefen as a volunteer commentedComment #54
tkoleary CreditAttribution: tkoleary commented@cilefen
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.
Comment #55
cilefen CreditAttribution: cilefen as a volunteer commentedOh, I thought it is twig... my bad.
It looks like some indentation has gone missing.
Comment #56
alexpottcore/modules/color/preview.html needs adjusting too if we're going to do this.
Comment #57
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@cilefen I'll see the Indentation again and
@alexpott , alright, i'm working on it.
Comment #58
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented56-and-Correcting-Indentation-1167366-58.patch
Contains both fixes: Indentation fix and Patch for #56
Correcting-Indentation-1167366-58.patch
Contains only correcting indentation.
Comment #59
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #62
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedWhy did this test fail? I just added a tab in the previous commit.
Comment #63
cilefen CreditAttribution: cilefen as a volunteer commentedYou must post the entire diff against the 8.3.x branch.
Comment #64
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #65
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #66
cilefen CreditAttribution: cilefen as a volunteer commentedIndentation is still changed in this patch.
Comment #67
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #68
cilefen CreditAttribution: cilefen as a volunteer commentedWhy is some text repeated in this section? It looks strange on a live site.
Why is some text ("This is a block of example content" and more) repeated in this section? It looks strange on a live site.
Comment #69
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedIt was from as suggested in #39
Any other alternative to change?
Comment #70
tkoleary CreditAttribution: tkoleary commented@cilefen
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?
Comment #71
cilefen CreditAttribution: cilefen as a volunteer commentedIt looks strange. It looks strange. See?
Comment #72
cilefen CreditAttribution: cilefen as a volunteer commentedI 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)
Comment #73
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedBut 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.
Comment #74
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedAny inputs on what should be done on this?
Comment #75
cilefen CreditAttribution: cilefen as a volunteer commentedI 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
Comment #76
tameeshb CreditAttribution: tameeshb at Google Summer of Code commented@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.
Comment #77
cilefen CreditAttribution: cilefen as a volunteer commentedI'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.
Comment #78
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedPatch with text modifications from #76
Please review & commit! :)
Comment #80
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #81
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedPlease review patch
Comment #83
ifrikTameeshb,
can you unassign yourself? Otherwise nobody else will pick this up to review it.
Comment #84
aroua_safa CreditAttribution: aroua_safa as a volunteer commentedI'm going to review it
Comment #85
aroua_safa CreditAttribution: aroua_safa as a volunteer commentedI made a review on 8.4.x versions and confirm that the patch work.
Comment #86
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #87
webchickNice! 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?
Comment #88
cilefen CreditAttribution: cilefen as a volunteer commentedExactly #53
Comment #89
xjmComment #90
Gábor HojtsyAll the way from 2011: :)
So why not make it a Drupal controller outputting that verbatim HTML with the translated pieces?
Comment #91
rakesh.gectcrI do agree Gabor for the same. would love to give it try
Comment #92
rakesh.gectcrThanks @Gábor Hojtsy, @manojapare and @lauriii for the support.
Comment #93
Gábor HojtsyThat 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 :)
Comment #95
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedRemoved javascript alert
Comment #96
rakesh.gectcr@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 .
Comment #97
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedOwh, sorry about that, but why was the alert put there? I didn't quite get that.
Comment #98
Gábor HojtsyHm, 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.
Comment #99
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #100
droplet CreditAttribution: droplet commentedI 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.
Comment #101
manojapare CreditAttribution: manojapare as a volunteer and at Valuebound commentedIMHO https://www.drupal.org/node/1167366#comment-11919727 we should not be removing the script tag because test is clearly checking for
So we should be sanitizing the html.
For that we can use
Xss::filterAdmin()
.Comment #102
rakesh.gectcr@Manoj Thanks for the help
Comment #103
drupalmonkey CreditAttribution: drupalmonkey commentedSecond tag should be "endtrans".
Comment #104
rakesh.gectcrComment #105
rakesh.gectcrComment #106
tkoleary CreditAttribution: tkoleary commentedLoos like all the trans tags are correct now.
Comment #107
droplet CreditAttribution: droplet commentedthis 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?
Comment #108
rakesh.gectcr@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
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 ofpreRenderInlineTemplate
.Comment #109
droplet CreditAttribution: droplet commented@see patch. This has twig template cached also.
Comment #110
rakesh.gectcr@droplet, Great, thanks for the patch. Looks like this good to go in. Making it to RTBC.
Comment #111
xjmComment #112
xjmThanks 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:
Is the use statement now unused?
Comment #113
cilefen CreditAttribution: cilefen as a volunteer commentedThere is a repeated sentence here.
Comment #114
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedAvoiding repetition from #113 with interdiff from #109
Comment #115
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedBefore:
After(applying patch on #114 ):
Comment #116
wturrell CreditAttribution: wturrell as a volunteer commentedReplaced
<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.)
Comment #118
wturrell CreditAttribution: wturrell as a volunteer commentedLook like 1167366-114.patch included a couple of unrelated contact form files. Let's see if it passes now…
Comment #119
wturrell CreditAttribution: wturrell as a volunteer commentedComment #121
aviseksingh CreditAttribution: aviseksingh at OpenSense Labs commentedNR #119
@wturrell The patch looks great. tested on 8.4.x-dev
Attaching screenshot before and after.
Comment #122
aviseksingh CreditAttribution: aviseksingh at OpenSense Labs commentedComment #123
wturrell CreditAttribution: wturrell as a volunteer commentedThanks. We still need tests for the translation.
Comment #124
Jeff Burnz CreditAttribution: Jeff Burnz commentedString 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?
is is
"Another" is not an ordinal number. Suggest something simple e.g. Tab One, Tab Two, Tab Three.
Ordinality not adhered to here either.
Comment #125
wturrell CreditAttribution: wturrell as a volunteer commentedRevised patch with Jeff Burnz's changes. (Skipping a further test run at this stage to reduce noise.)
Comment #126
Jeff Burnz CreditAttribution: Jeff Burnz commentedMore nit picking...
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:
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.
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.
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
Comment #127
swarad07Made changes as per feedback in #126
Comment #128
Jeff Burnz CreditAttribution: Jeff Burnz commentedGreat stuff @swarad07, looking much better, just a couple more nit picks (yes sorry I am that guy :)
Indent incorrect.
img tag on new line
Comment #129
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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.
Comment #130
Gábor HojtsyAgreed, let's remove the whitespaces at start/end of trans.
Comment #131
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedApplying 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?
Comment #132
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #133
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis one I think like this:
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 :)
Comment #134
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrections from code review by @Jeff Burnz.
Comment #135
Jeff Burnz CreditAttribution: Jeff Burnz commented#134 looks good.
#112 to go.
Comment #136
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #137
Bojhan CreditAttribution: Bojhan as a volunteer commentedAwesome, I am looking forward to seeing the latin gone :)
Comment #138
rakesh.gectcr@Jo Fitzgerald, I think patch [#134] having unwanted changes.
Comment #139
rakesh.gectcrPlease check the patch and inter diff files
Comment #140
rakesh.gectcrComment #141
wturrell CreditAttribution: wturrell as a volunteer commentedI'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.
Comment #142
droplet CreditAttribution: droplet commentedSplit the translation is bad. That's paragraph and we should not force translate sorting in that ordering.
For example:
In other languages, it may translate into
Or
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.
Comment #143
rakesh.gectcrComment #144
rakesh.gectcrDone 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
Any one has any clue about this please let me know
Comment #146
rakesh.gectcrI 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
Comment #148
rakesh.gectcrWell, there was a typo on test.
Comment #149
rakesh.gectcrComment #150
mikeoharaI am going to visually test this as part of DrupalCon Baltimore 2017
Comment #151
mikeoharaTested this and it seems to be behaving as expected in 8.4.x
Comment #152
mikeoharaComment #153
star-szrThanks everyone for the work on this so far :)
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.
Minor: Twig is not acronym so this should be "Twig" instead of "TWIG".
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...
Similarly here.
Minor: Extra blank line at the bottom of the setUp method.
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.
Comment #154
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedI'll work on the feedback from @Cottser.
Comment #155
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedHere 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!
Comment #157
c.nish2k3 CreditAttribution: c.nish2k3 as a volunteer commentedFixing the error
Comment #158
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedHere is in interdiff of 155 to 156.
Noticed the following in 156:
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.:
Comment #159
rakesh.gectcrThe 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?Comment #160
Gábor HojtsyAgreed with @rakesh.gectcr from #159:
Yeah this does not really ensure anything except that at least one opening and one closing trans tag exists in the file.
Comment #161
wturrell CreditAttribution: wturrell as a volunteer commented@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...
Comment #162
Gábor HojtsyThere 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.
Comment #163
star-szrThank you @Gábor Hojtsy for that info and sorry if my comment caused any confusion/unnecessary churn.
Comment #164
c.nish2k3 CreditAttribution: c.nish2k3 as a volunteer commentedWhat is the best way to check this? The output is going to be in a language whihc we do not know now.
Comment #165
droplet CreditAttribution: droplet commentedFind a way out: #2881125: How to format long translatable string on Twig (PHP)?
Comment #166
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #167
neerajpandey CreditAttribution: neerajpandey at Google Code-In commentedComment #168
neerajpandey CreditAttribution: neerajpandey at Google Code-In commentedComment #169
TR CreditAttribution: TR commentedI 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.
Comment #170
rakesh.gectcrWe need to address #2881125: How to format long translatable string on Twig (PHP)? first.
Comment #171
rakesh.gectcrHi I am working on the test, the following is the code, I got struck on translation the following is the initial code.
Need Guidance on
Comment #172
rakesh.gectcrLooks like I figured it out. :) Kindly review it.
Comment #173
dawehnerThank you for working on the test!
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
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()
Comment #174
rakesh.gectcrComment #175
rakesh.gectcrThanks @dawehner for the review.
protected $profile = 'standard';
We need to have Bartik theme installed for testing
Drupal\Tests\language\Functional\LanguageLocaleListTest;
Comment #176
rakesh.gectcrTalked to @dawehner and @phenaproxima
need to Change code accordingly.
Comment #177
rakesh.gectcrMentioned all the above comments,
Please review...
Comment #178
dawehnerThis 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.
Comment #179
lauriiiThe name of the theme should not be translatable. This could be super confusing for a translator.
We should make the order of the sentences consistent with color module.
This should also be just a single trans block.
The alt text should be translatable
Comment #180
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commented#1 done, #2 and #3 I made like color module with h2 and p, #4 done
Comment #181
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedComment #182
vegantriathleteYou can see about getting this to RTBC and / or creating an issue summary.
Comment #183
greggmarshall#DCCO2017 working on this
Comment #184
greggmarshallReviewed 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.
Comment #185
greggmarshallComment #186
rakesh.gectcr@greggmarshall Thank you for making it o RTBC. +1
Comment #187
lauriiiThese should be formatted so that the line length doesn't exceed 80 characters.
Comment #188
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedfixed as per #187
Comment #189
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #190
vegantriathleteComment #192
rakesh.gectcrLooks like @laurriii's comment has been applied. Moving back to RTBC
Comment #193
lauriiiCould we use the short syntax for this instead so we don't have to split this to 2 lines?
There's still lines that exceed the maximum line length
There's unneeded space before the first word.
Comment #194
rakesh.gectcrWorking on this
Comment #195
rakesh.gectcr@lauriii
Comment #196
rakesh.gectcrComment #197
jofitz CreditAttribution: jofitz at ComputerMinds commentedThere are still a couple of sections that don't fill the 80 characters correctly.
Comment #198
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedJo Fitzgerald -> 80 characters set correctly
Comment #199
rakesh.gectcrLooks good, Moving to RTBC
Comment #200
larowlanAre 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.
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.
Comment #201
joelpittetI 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.
Comment #202
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #203
MaskyS CreditAttribution: MaskyS at Google Code-In commentedRemoved an old tag :)
Comment #205
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedAdding test in patch #198 for version 8.6.x :)
Comment #206
joelpittetSo the tasks:
Comment #207
benjifisherComment #208
jphelan CreditAttribution: jphelan at Devvly commentedReview 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.
Comment #209
jphelan CreditAttribution: jphelan at Devvly commentedComment #210
benjifisher@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.
Comment #211
jphelan CreditAttribution: jphelan at Devvly commentedSorry, the last patch was missing some of the changes. This is the correct patch.
Comment #212
jphelan CreditAttribution: jphelan at Devvly commentedYes, here is the interdiff.
Comment #215
jphelan CreditAttribution: jphelan at Devvly commentedReroll patch for failed test.
Comment #217
rakesh.gectcrComment #218
tresti88Comment #223
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedRe-rolling #198 patch for 9.1.x
Comment #224
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedIt's in idle state from long. I'm moving it in review.
Comment #225
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedComment #226
narendra.rajwar27Comment #227
narendra.rajwar27Re-rolled the patch for 9.1.x.
Comment #229
narendra.rajwar27Comment #231
narendra.rajwar27Fix for failed test cases.
Comment #232
narendra.rajwar27Comment #236
Asha Nair CreditAttribution: Asha Nair at Zyxware Technologies commentedPatch #231 failed to apply
Comment #237
benjifisherI 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.
Comment #238
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #231.
Comment #239
andregp CreditAttribution: andregp at CI&T commentedFixed Coding Standards on patch #238 (Custom Commands Failed test)
Edit: all right, my patch failed too :/
Comment #240
andregp CreditAttribution: andregp at CI&T commentedSending back to Needs Work as it failed testing.
I'll work on this.
Comment #241
andregp CreditAttribution: andregp at CI&T commentedOkay, 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.
Comment #242
TR CreditAttribution: TR commentedUsing @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.
Comment #243
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedComment #245
andregp CreditAttribution: andregp at CI&T commentedThank you @TR for the tip and thanks @ranjith_kumar_k_u for fixing it!
New patch (as #243 got some failed tests).
Comment #246
andregp CreditAttribution: andregp at CI&T commentedI forgot to change status.
Comment #248
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed test of patch #245.
Comment #249
andregp CreditAttribution: andregp at CI&T commented@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)
Comment #250
TR CreditAttribution: TR commentedYou 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?
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.
Comment #251
andregp CreditAttribution: andregp at CI&T commentedSo, 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 usedresponseContains('<h2>Example heading</h2>')
instead ofresponseNotContains('<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.moduleAfter 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.
Comment #252
andregp CreditAttribution: andregp at CI&T commented@TR
Regarding you question on #250.2
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.
Comment #253
andregp CreditAttribution: andregp at CI&T commentedSome more fixes based on comment #250 (thanks @TR)
Comment #254
Johnny Santos CreditAttribution: Johnny Santos at CI&T commentedI'm going to apply the patch and see if its working as planned to be.
Comment #255
Johnny Santos CreditAttribution: Johnny Santos at CI&T commentedapplied the patch presented on comment #253.
its working as intended for the preview on bartik theme.
(sent the same picture twice, sorry)
Comment #256
Johnny Santos CreditAttribution: Johnny Santos at CI&T commented+1 RTBC
Comment #258
Shiv Yadav CreditAttribution: Shiv Yadav commentedI have tasted on drupal 9.5.x-dev version and patch is applying properly
Patch #253 is working properly
Comment #259
xjmThanks 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.
This has different indentation from
preview.html.twig
and is being wrapped at about 83 characters instead of 80.a Twig file, not "an".
This isn't quite a sentence. Maybe:
Nit: This should say either "Add French" or "Add the French language".
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.")
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.
This should say "translated to French" (not "translating").
Comment #260
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedMade changes as per comment #259, please review.
Comment #262
quietone CreditAttribution: quietone at PreviousNext commentedColor has been removed from core, #3270899: Remove Color module from core.
Comment #263
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded patch #260 on Color module.