Follow up for #1940590: META: Write a multipage multilingual tour.

Problem/Motivation

Multilingual configuration is spread throughout the site. It would be useful to have a tour to help users know how to configure the site for multilingual. There should be individual tours for certain multilingual features and these can be linked together to make a multipage tour later via #1940590: META: Write a multipage multilingual tour.

Proposed resolution

Write individual tours for the language section only.

Remaining tasks

Create tour yml files for the language tour:

  • Languages page (admin/config/regional/language)
    • Add new language (subpage = admin/config/regional/language/add)
    • Edit a language (subpage = admin/config/regional/language/edit/[langcode])
    • Reorder languages

User interface changes

New tours

API changes

None

Technical pointers when creating tour tips

See: https://drupal.org/node/1921152#tour-tips-tech-note for tech notes on making tour tips.

How to test

  • Use latest Drupal 8 and apply latest patch below (or use simplytest.me button next to patch)
  • Enable Tour and Language modules (make sure to uninstall Language module if it was enabled)
  • Go to: admin/config/regional/language
  • Click on tour icon on upper right (don't see it? clear your cache at: admin/config/development/performance)
  • Walk through first tour
  • Click "Add language" button
  • Click on tour icon on upper right
  • Walk through second tour
  • Go to: admin/config/regional/language
  • Click "edit" on a language
  • Click on tour icon on upper right
  • Walk through third tour
  • Provide feedback in a comment :)

Related issues

#1921152: META: Start providing tour tips for other core modules.
#1942576: Tour tips to be able to link to other pages and start tour's automatically.
#1940590: META: Write a multipage multilingual tour

Files: 
CommentFileSizeAuthor
#171 interdiff.txt2.03 KBbalagan
#171 2017471-tour-lang-171.patch13.05 KBbalagan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,488 pass(es). View
#169 interdiff.txt1.03 KBrodrigoaguilera
#169 2017471-tour-lang-169.patch13.07 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,210 pass(es). View
#160 interdiff.txt2.92 KBrodrigoaguilera
#160 2017471-tour-lang-160.patch13.04 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,051 pass(es). View
#158 interdiff.txt11.88 KBrodrigoaguilera
#158 2017471-tour-lang-158.patch13 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,955 pass(es). View
#156 interdiff.txt3.18 KBrodrigoaguilera
#156 2017471-tour-lang-156.patch9.94 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,697 pass(es). View
#155 2017471-tour-lang-155.patch9.9 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,643 pass(es). View
#152 2017471-152.patch36.78 KBrpayanm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,570 pass(es), 2 fail(s), and 0 exception(s). View
#152 2017471-interdiff.txt1.48 KBrpayanm

Comments

Kristen Pol’s picture

Assigned: Unassigned » Kristen Pol

I'm going to play with this.

Kristen Pol’s picture

Ok... they are pretty easy to make. The annoying thing is that you have to clear the cache and reload the page every time you make a change. Also, the yml file needs to be in the *active* config directory so it gets picked up. I'm not sure the proper way to do this so I just copied my file there for now. The active directory is something like:

sites/default/files/config_PRC-r12K-N94eODx1_m4Dg1Q9kpXtC8jLEknhw5JmOI/active

So obvious! :)

The only other annoying thing is that there isn't always good markup to target exactly where you'd like the tool tip to show up. I will play with this some more.

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

Kristen Pol’s picture

Component: locale.module » language.module
Assigned: Kristen Pol » Unassigned
Status: Active » Needs review
Issue tags: +Novice, +Needs manual testing
FileSize
7.09 KB
FAILED: [[SimpleTest]]: [MySQL] 57,525 pass(es), 2 fail(s), and 0 exception(s). View

Here's a first pass. There are 3 tours:

  • Languages page
  • Languages add page
  • Languages edit page

I'm looking for feedback on:

  • Tips that should be added
  • Tips that should be removed
  • Wording

In general, I took this approach:

  • Overview tip
  • Tip for each of the major features on the page
  • Closing tip that can point off to other pages if it makes sense

One thing to note: I added some links in the tips but these are relative to the domain and don't include any base path info... for example, I'm running my site locally at 127.0.0.1:8080/d8dev/index.php/[blah-blah] but the URLs end up being 127.0.0.1:8080/[blah-blah]. This is fine if all sites are run at the top level but that is not always the case. I'm not sure how to specify in the tour tip URL how to tack on the URL base path.

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

Kristen Pol’s picture

Issue tags: -Novice, -Needs manual testing

I added instructions on how to test the tours in the issue summary.

Kristen Pol’s picture

Issue tags: +Novice, +Needs manual testing

Didn't mean to remove tags!

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, drupal8.language-tours.2017471-3.patch, failed testing.

Gábor Hojtsy’s picture

I ran through all three tours.

Are there any guidelines posted for tours? I see you made up some great things, but it would be great to conform to some common pattern. Even if we set that pattern up just now :D

I think the overview tour tip is a good idea and the next steps tip also sounds good. The tip on the operations and the save button on forms seems like a bit excessive :) Those are repeating elements throughout the site, so tour tips for them may become tiring pretty quickly.

clemens.tolboom’s picture

The language add tour end on 6 out of 8 ( in reality we have just 7 steps)
57.png

This one is skipped.

languages-edit-english-trans:
    id: languages-edit-english-trans
    plugin: text
    label: Allowing English translation
    body: <p>By default, interface strings bundled with Drupal are in English. If you wish to change these English interface strings, enable this checkbox.</p><p>The "Interface Translation" module provides this feature.</p>
    weight: "5"
    attributes:
      data-id: edit-locale-translate-english
nick_schuch’s picture

Hi clemens.tolboom,

Can post your yml file and I will take a look and see why we are gettting 6 of 8 on the last tip?

Gábor Hojtsy’s picture

BTW re the links, I think tour module should provide a text filter to form links to the proper local places. It may already do so.

nick_schuch’s picture

Im not sure I follow Gabor?

clemens.tolboom’s picture

@nick_schuch I just did $ drush iq-apply-patch 2017471.

nick_schuch’s picture

oh! A patch in #3! I will also take a look first thing tomorrow morning.

Kristen Pol’s picture

Thanks for testing everyone!!!

Re #7:

I don't know of any guidelines for tour content or structure. The Tour API doc just explains how to use it. How would we go about creating "tour tip guidelines" doc? Would it just be an issue, or in the handbooks?

I personally really like the overview and closing tips. I don't know if others will want to adopt the same pattern but I think it is nice to give a quick summary of the page and then the closing "what you can do now". Once the multipage tours are working, the links can point off to the next tour on a different page.

I looked at the screenshots #1921152-29: META: Start providing tour tips for other core modules. and see that they have a "save changes" tip. Since this page doesn't have a lot of languages, then the "save" is pretty obvious and could be rolled into the tip for reordering.

The only reason *maybe* to have the operations one is that is it obvious that they click on it to get the "delete" option? Also, we can put the reason that the "delete" doesn't show up for the default language.

Those are repeating elements throughout the site, so tour tips for them may become tiring pretty quickly.

True. So... maybe these can be addressed on the overview tip? But, too many things on the overview and then they won't read it because it will become a wall of words ;)

I think in the case when the save button is far away from the rest of the page actions (beyond the "fold") then having the save tip makes sense. The operations one may or may not make sense... maybe it would make sense if you can give some insight into what happens when you use the operations.

Re #8:

This was an interesting issue (and bug, I think, with Tour). I added that tip to show up on the English language "edit" because the checkbox appears there. It does not appear for other languages. If you run the tour on the English edit page, you get 7 tips which is correct. If you run it on a non-English edit page, it says 8 tips (and it skips the checkbox because it's not there) so you end up with 6 out of 8.

I would need to understand how you *conditionally* add tips. What if there is markup that is hidden on the page and they click it open? Or, there are different options on the page depending on what you are editing. How do you handle this sanely in the tour? I don't know.

For now, I could leave out the English checkbox tip, but it is actually a very important checkbox that might need some extra attention.

Regarding #10:

It would be great to know how to put in the links :) Maybe @nick_schuch has some thoughts on that.

Thanks again everyone! This is fun stuff and I'm excited about this getting into D8!

larowlan’s picture

re #10 I believe Gabor is advocating that tour can markup links automatically, if we wire them with /foo/bar and someone uses the site at /drupal/foo/bar it won't work.

But this would need its own syntax

something like {link|Continue next tour|foo/bar} could be parsed and output as l('Continue next tour', 'foo/bar')

Thoughts?

Kristen Pol’s picture

@larowlan Something like that would be awesome! Is it already supported or do we need a patch to [something] to support? If the latter, what do we patch?

nick_schuch’s picture

Well need a filter of some description for this. Ill look at other parts of core and see if we are tackling it anywhere else.

clemens.tolboom’s picture

I just learned tour has a _test_ plugin TipPluginImage and only a TipPluginText for real.

The TipPluginText calls: filter_xss_admin($this->getBody())

My guess is we can use tokens for this and only have to patch TipPluginText

Kristen Pol’s picture

Thanks @clemens.tolboom for looking into this... hmmm... I'm not sure if I know enough about the plugins and best tokens to use to attempt a patch for TipPluginText... any takers? :)

Gábor Hojtsy’s picture

@Clemens: indeed, I think that is the right place to introduce this feature.

@Kristen: I think some guidelines for how to write tips would be good, we should start an issue AND a documentation page. So if there are conflicting viewpoints, we can use the issue to track ideas. Interestingly tour writing did not pick up as much as people hoped even though there was a big push to get in the tour module itself, it keeps being underutilized...

clemens.tolboom’s picture

larowlan’s picture

Yeah, we didn't get as much interest in writing tours as we hoped :(
Given they're essentially strings, I wonder if they're still ok after July.

Gábor Hojtsy’s picture

I think they are still ok after July for some time. Not sure how long, there are no defined points after that yet.

clemens.tolboom’s picture

I just did (party) the tour for #1920468: Write a tour for the first page after install showing extend and other things which was not that hard but still not trivial. The instructions on #1921152: META: Start providing tour tips for other core modules. are OK but 'is there a module for this'? would help UX people. `Me wants code`

Gábor Hojtsy’s picture

#2019469: Tour module should use token for it's body is fully solved, so links can now be made in tours :) Just use: <a href="[site:url]admin/config/...">...</a>

Kristen Pol’s picture

Yep!!!! Now I have no excuse to not finish ;)

Kristen Pol’s picture

Regarding Tour documentation, I just searched and found:

So the most guidelines are on:

https://drupal.org/node/2000088

under "general principles" section.

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
7.91 KB
FAILED: [[SimpleTest]]: [MySQL] 58,478 pass(es), 0 fail(s), and 7 exception(s). View

Here are the updated tours. I removed some tips (mostly consolidation) and add an alter function that adds in some extra info based on if the interface translation and content translation modules are enabled.

Status: Needs review » Needs work

The last submitted patch, drupal8.language-module-tours.2017471-28.patch, failed testing.

Kristen Pol’s picture

The test result doesn't make sense to me since the argument appears to be correct in language_tour_tips_alter... re-running the tests.

Kristen Pol’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs manual testing, -D8UX usability, -D8MI, -language-base, -Tour

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +D8UX usability, +D8MI, +language-base, +Tour

The last submitted patch, drupal8.language-module-tours.2017471-28.patch, failed testing.

David Hernández’s picture

Status: Needs work » Needs review
FileSize
7.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,689 pass(es). View

Removing the requirement of the second attribute of the function language_tour_tips_alter to be an EntityInterface solved the issue, however, I'm not sure if this is the right solution.

Kristen Pol’s picture

Thanks David... interesting... I just copied the function signature from the tour_test module... Hmmm...

nick_schuch’s picture

Hi Kirsten,

I see you are using language_tour_tips_alter() to check if locale module is enabled to add additional overview.

Could the additional overview be separate tips? That way they will be displayed if a specific element is available on the page.

larowlan’s picture

+++ b/core/modules/language/config/tour.tour.language.ymlundefined
@@ -0,0 +1,49 @@
+  - admin/config/regional/language ¶

Whitespace

+++ b/core/modules/language/language.moduleundefined
@@ -848,3 +848,41 @@ function language_system_regional_settings_form_submit($form, &$form_state) {
+function language_tour_tips_alter(array &$tour_tips, $entity) {

should type hint EntityInterface, requires you 'use \Drupal\Core\Entity\EntityInterface' at the top of the file
that will fix the warnings.

Kristen Pol’s picture

@nick_schuch - I'm not sure I follow:

Could the additional overview be separate tips? That way they will be displayed if a specific element is available on the page.

Oh... maybe I do. There is one check to see if locale is enabled so that it will decide to show a message about the interface translation numbers showing up on the page.

Is there a way in tours for a tip to show up *only* if the element is available on the page? On the edit page, I removed a tip because in one case (English language), it shows a checkbox and for other languages it does not. When I included a tip about the checkbox, the tip numbers were *wrong* at the bottom of the tips... e.g. there were 7 tips including the checkbox tip. For English, it was correct, 1 out of 7, 2 out of 7... 7 out of 7 (last tip). For other languages, it was wrong... 1 out of 8 ... 6 out of 8 (last tip!).

If there is a way to conditionally include tips if markup is present, that would make things easier. Maybe the above example is just highlighting a bug in Tour? Or in Joyride?

Kristen Pol’s picture

FileSize
543 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,144 pass(es). View
1.18 KB

I updated the patch with changes from #36. Thanks @larowlan !

larowlan’s picture

@KristenPol, the js will remove tips when their selector element isn't found on the page

Kristen Pol’s picture

Thanks @larowlan. I found that it was buggy when it had that scenario (see comment #37). So... should I just add the tips in assuming that works as expected and that issue can be filed as a tour bug?

I'm worried that this issue can't get RTBC'ed with that bug I mentioned showing up.

clemens.tolboom’s picture

@Kristen Pol pathc from #38 is empty. Can you please add a new patch.

Regarding the counter I've seen that now and then.

Will try to write a test for it in #2028535: Provide a TourTestBase class for use by core and contrib modules

Outi’s picture

FileSize
8.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,324 pass(es). View

@Kristen Pol I applied the patch from #33. The interdiff file didn't apply. I manually added into it the changes documented on the interdiff file of the #38, resulting this patch number 42 that should be similar to what you wanted on 38.

Outi’s picture

This is the interdiff file between the patch on the #33 and the patch on the #42.

Outi’s picture

This is not worth making a new patch on its own, but if someone is changing something else in the future, put this one in alphabetical order:

+++ b/core/modules/language/language.moduleundefined
@@ -7,6 +7,7 @@
 use Drupal\node\NodeTypeInterface;
 use Drupal\Core\Language\Language;
+use Drupal\Core\Entity\EntityInterface;
clemens.tolboom’s picture

#35 and #37 are still not 'addressed'.

My problem with the use of hook_tour_alter lies in code interfering with the tour. Tour text is not anymore within a tour only. The translation workflow is very weird now.

In #35 @nick_schuch suggest to add more steps but that is only possible when the page contains something like a class body.module-no-locale.module-no-entity-translate to hide the dialog when those module are not enabled.

Guess token is not useful for this conditional behavior?

Outi’s picture

I tested this tour as a user. It's pretty clear but I have a couple of suggestions.

- On the second window, it is said "To add more languages to your site, click the "Add language" button."

So I do: I click the button and add a language. Once the language imported, I see it on the list with the indication "0/232 (0%)" on the Interface translation column. That's normal but frustrating: as a user I might expect that once I've downloaded the language, I have the language and I don't need to translate anything.

So perhaps we should mention on the tour what it means to "add a language"; now, as an uninitiated used, I would be frustrated about not to know what to do and how to use my new language. All this should of course be explained shortly, the purpose being to reassure the user that everything is going fine.

(But I don't have any suggestion yet about how and at where to explain it!)

- On the third window, there is this paragraphe that is not clear to me:

"This order will determine the order in which languages are displayed in lists on the site such as in the language switcher blocks provided by the "Interface Translation" and "Content Translation" modules."

Are there several language switcher blocks that are provided by the Interface Translation and Content Translation modules ? If yes, is it useful to mention the modules? Could it be only "[--] such as in the language switcher blocks." If not, should it be reformulated, e.g. "[--] such as in the language switcher block or other blocks provided by the "Interface Translation" and "Content Translation" modules."

I would also remove the first "order" as it appears also in the first paragraph and later in the same sentence, and say: "This will determine the order in which languages are displayed [--]"

Kristen Pol’s picture

@clemens.tolboom - Thanks for the feedback. Yeah... I had a note in #40 about what to do about the extra tips... sounds like I should just add them and hope all gets fix with the underlying "page N of M" issue which you noticed in #8.

It sounds like it's better for translation reasons to not use the language_tour_tips_alter to add in the extra tips body text... that is a shame (since it's kind of cool that it is "smart") but I understand the reasoning.

Can I add a new css class into the body in the language_tour_tips_alter so that my extra tips are triggered off of that? Not sure how that would work in D8.

@Outi - good stuff, thanks!

1) I agree that some more explanation would be good but not sure myself what makes sense ;) I'm a little puzzled though because I thought when you added the language it was supposed to automatically get the interface translations so maybe something is broken? Maybe there is an issue for that...

2) I also agree that just using "language switcher blocks" (by itself) is fine... don't need to call out the modules here. Right now there are 2 blocks but I think that they are trying to make it so there is only one (which makes more sense!). There should be an issue for that somewhere... so... maybe just mention "language switcher block" (singular) since you only use one at a time.

Outi’s picture

Kristen Pol:

I'm a little puzzled though because I thought when you added the language it was supposed to automatically get the interface translations so maybe something is broken? Maybe there is an issue for that...

Ah, perhaps it isn't normal then :). Actually, if you install the site in French for example, on the translation interface http://localhost:8888/drupal/admin/config/regional/translate/translate some of the strings are translated, some others aren't:

tranlation-ui-siteEN-languageFR.jpg

Now, if you install the site in English and add the French language on the language page, on the translation interface everything is empty and the proposed strings are not the same in the French installation:

tranlation-ui-siteFR-languageFR.jpg

So perhaps something is indeed broken, or perhaps the language files are not the same in these two cases.

c-c-m’s picture

Status: Needs review » Reviewed & tested by the community

Applyed last patch and it worked fine (when using English interface only -in other languages the tour icon didn't show up).

The tour is properly display and I found the texts to be very helpful. Also links worked and are made relative to site's url (great!) and the number of slides (6) seem to be correct.

I'm tagging this as RTBC.

Thanks for your work!

Kristen Pol’s picture

Thanks for the review @c-c-m :) I think there is still some work to do per #47.

I'll try to ping @clemens.tolboom to see how best to proceed.

clemens.tolboom’s picture

The hook_tour_alter is a weird feature of tour.module so guess we have to live with it :-/

I hope #1959660: Replace xpath() with WebTestBase::cssSelect() by leveraging Symfony CssSelector and then #2028535: Provide a TourTestBase class for use by core and contrib modules get's in too so we can really write tests easily for tours.

Apart from the tiny issues below it thinks it's RTBC enough.

+++ b/core/modules/language/config/tour.tour.language-edit.yml
@@ -0,0 +1,49 @@
+    body: <p>This page provides the ability edit a language on your site including custom languages.</p>

the ability _to_ edit a language

+++ b/core/modules/language/config/tour.tour.language-edit.yml
@@ -0,0 +1,49 @@
+    body: <p>Choose if the language is a "Left to right" or "Right to left" language.</p><p>Note that not all themes support "Right to left" layouts so test your theme if you are using this directionality.</p>

we are always _using_ a directionality so that could use a rephrase.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Moving to 'needs work' because of #51. Needs a quick re-roll and then I think we're good to go.

+++ b/core/modules/language/config/tour.tour.language.ymlundefined
@@ -0,0 +1,49 @@
+    body: <p>To reorder the languages on your site, use the drag icons next to each language.</p><p>This order will determine the order in which languages are displayed in lists on the site such as in the language switcher blocks provided by the "Interface Translation" and "Content Translation" modules.</p><p>If you reorder the languages, click the "Save configuration" button when you are done for the changes to take effect.</p>

We don't camel-case each word in a title (e.g. 'Interface Translation' should be 'Interface translation'). May have to be fixed throughout Drupal.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
40.92 KB
4.18 KB
8.07 KB
PASSED: [[SimpleTest]]: [MySQL] 56,360 pass(es). View

I've applied feedback from #51 and #52.

I ran again into the hook_tour_tips_alter as CamelCase text needed to change there too. That is not cool :-/
hook_tour_tips_alter.png

Follow-up issue: #2031607: hook_tour_tips_alter is kinda blocking the Tour writer process

Also note we still needs tests but let's create those after #1959660: Replace xpath() with WebTestBase::cssSelect() by leveraging Symfony CssSelector and then #2028535: Provide a TourTestBase class for use by core and contrib modules lands as those will ease the burden of writing tour specific tests.

Kristen Pol’s picture

Well, there was an issue to make all *module* names Camel Case. In this case (ha), I'm referring to the *modules* and that is why I intentionally did Camel Case. But, if I was to just refer to the feature then I wouldn't.

So... I'm unclear if this really does need changing.

Kristen Pol’s picture

nick_schuch’s picture

Issue tags: +Needs tests

Tour needs tests since we now have #2028535

kreatIL’s picture

Issue summary: View changes

add more instructions for testing

kreatIL’s picture

Attending the mentored Core Sprint at DrupalCon Prague 2013.

The tour icon doesn't show up to me at all. I took patch #53, applied it to the latest d8 and followed every single step described above.

I also switched to English interface as default, which was mentioned in #49.

clemens.tolboom’s picture

@kreatIL I just applied patch from #53 then had to uninstall the language module first to get the icon in place. (the reason is language/config/tour.tour.* files are copied to files/config_*/active on install only ; you may also visit update.php maybe)

+++ b/core/modules/language/config/tour.tour.language-add.yml
@@ -0,0 +1,31 @@
+    body: <p>Now that you have an overview of the "Add languages" feature, you can continue by:<ul><li>Adding a language</li><li>Adding a custom language</li><li><a href="[site:url]/admin/config/regional/language">Viewing enabled languages</a></li></ul></p>

please make this autostart and also other links to Tours.

clemens.tolboom’s picture

FileSize
9.72 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.language-module.2017471-59.patch. Unable to apply patch. See the log in the details link for more information. View

I've added the tests ... which will fail on two tips on the admin/config/regional/language I somehow don't get.

Nothing has been changed on the tour.tour.* files.

@kreatIL I'll fix your patch in case you want to upload one :)

kreatIL’s picture

Patch #59 worked for me. Before applying the patch I had to uninstall the Interface Translation module at first and the Language module right afterwards. After re-installing the Language module and clearing the cache, the tour icon showed up properly.

All three tours worked fine, without any errors (FF 23/Mac and also Chrome 29/Mac). Quite a nice feature :)

clemens.tolboom’s picture

@kreatIL you could try to test the peculiar behavior by enabling the locale and translation_entity modules as the tips are changed on their availability.

We still wait for the testbot to finish but it seems to be reset whatever that means :-/

https://qa.drupal.org/pifr/test/637708 Test reset by client request. 25 min ago

Status: Needs review » Needs work

The last submitted patch, drupal8.language-module.2017471-59.patch, failed testing.

clemens.tolboom’s picture

The failures are

Found corresponding page element for tour tip with class .action-links	Other	LanguageTourTest.php	50	Drupal\language\Tests\LanguageTourTest->testLanguageTour()	
Found corresponding page element for tour tip with class .dropbutton-action	Other	LanguageTourTest.php	50	Drupal\language\Tests\LanguageTourTest->testLanguageTour()

but the elements are available when running the test locally. Hope someone will try this locally.

clemens.tolboom’s picture

How to test:

- on the test results verbose output page search for jQuery('.action-links'). It exists.

The only difference between the data-class .draggable and .action-links is the '-' ... to test for this I've probed XML.php

    public static function cssSelect($selector, $content, $actual, $isHtml = TRUE, $tester = NULL)
    {
        $matcher = self::convertSelectToTag($selector, $content);
        if ($tester) $tester->vverbose('Matcher: ' .print_r($matcher, TRUE));
        $dom     = self::load($actual, $isHtml);
        $tags    = self::findNodes($dom, $matcher, $isHtml);
        if ($tester) $tester->vverbose('TAGS: ' . print_r($tags, TRUE));

        return $tags;
    }

and TourTestBase.php with

  public function vverbose($x) {
    $this->verbose($x);
  }

That did not help :-/

[EDIT]
and of course the call to cssSelect for assertTourTips()

        else if (!empty($tip['data-class'])) {
          $elements = \PHPUnit_Util_XML::cssSelect('.' . $tip['data-class'], TRUE, $this->content, TRUE, $this);

[/EDIT]

larowlan’s picture

Php unit's xml select helper doesn't support several html 5 elements, Nick had been in conversation with them as it is going to be fixed in 3.8. There is an issue to update our version once that is released. In the meantime, an alternate selector might be possible?

larowlan’s picture

Issue summary: View changes

Added uninstall hint.

willieseabrook’s picture

The last submitted patch, 59: drupal8.language-module.2017471-59.patch, failed testing.

willieseabrook’s picture

Issue summary: View changes
Issue tags: +SprintWeekend2014, +TUNIS_2014_JANUARY

Starting as part of DrupalSprintWeekend

willieseabrook’s picture

Assigned: Unassigned » willieseabrook
oussema’s picture

Assigned: willieseabrook » oussema

Starting work

oussema’s picture

Assigned: oussema » Unassigned
Status: Needs work » Needs review
FileSize
10.13 KB
FAILED: [[SimpleTest]]: [MySQL] 63,246 pass(es), 3 fail(s), and 0 exception(s). View

Rerolled the patch against head

larowlan’s picture

Status: Needs review » Needs work

The last submitted patch, 71: drupal8.language-module.2017471-71.patch, failed testing.

danylevskyi’s picture

Assigned: Unassigned » danylevskyi
danylevskyi’s picture

Assigned: danylevskyi » Unassigned
Status: Needs work » Needs review
FileSize
9.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,819 pass(es), 5 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 75: drupal8.language-module.2017471-75.patch, failed testing.

danylevskyi’s picture

Status: Needs work » Needs review
FileSize
9.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,767 pass(es), 3 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 77: drupal8.language-2017471-82.patch, failed testing.

danylevskyi’s picture

Assigned: Unassigned » danylevskyi
danylevskyi’s picture

Assigned: danylevskyi » Unassigned
Status: Needs work » Needs review
FileSize
9.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,785 pass(es). View
balagan’s picture

Few notes and suggestions to change:
I think the following could be better organized:
"If you reorder the languages, click the "Save configuration" button when you are done for the changes to take effect."

suggestion:
"When you are done with reordering the languages, click the "Save configuration" button for the changes to take effect."

"You can edit the language's name, code, and directionality."
suggestion:
"You can edit the name, code and direction of the language."

"Note that you cannot delete the site's default language."
suggestion:
"Note that you cannot delete the default language of the site."

"You will get an additional form when adding a custom language where you can provide the language's name, code, and directionality."
suggestion:
"When adding a custom language, you will get an additional form where you can provide the name, code and direction of the language."

"You cannot change the language's code on the site since it is used by the system to keep track of the language."
suggestion:
"You cannot change the code of the language on the site, since it is used by the system to keep track of the language."

"Note that not all themes support "Right to left" layouts so test your theme if you are using "Right to left"."
suggestion (add comma):
"Note that not all themes support "Right to left" layouts, so test your theme if you are using "Right to left"."

balagan’s picture

Status: Needs review » Needs work
rak2008’s picture

Status: Needs work » Needs review

I reviewed and apply the patch with enabling the Language module and i test add language and edit the language.
And i uninstall the Language module and it did not show me the languages menu.

rak2008’s picture

Status: Needs review » Needs work

I did see the last comment And i thank it need work

balagan’s picture

FileSize
9.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,920 pass(es). View

Sorry, I work on Windows, and could not find an interdiff tool in a realistic timeframe.
Sending a patch with my suggestions.

balagan’s picture

Status: Needs work » Needs review
rak2008’s picture

Issue tags: -Needs manual testing

I reviewed the patch #2017471-85 And i enabled the Language module after that i check (admin/config/regional/language) and i click on tour icon and walk through
the first tour And it's work good and I click on Add language link and it's work good.
And click on tour on add language and work fine.

thlafon’s picture

FileSize
9.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,654 pass(es). View
1.27 KB

I reviewd drupal8.language-2017471-85.patch and it worked as expected on the 3 differents paths
admin/config/regional/language
admin/config/regional/language/add
admin/config/regional/language/edit/xxx

The only thing i modified was a spelling mistake on the word "don" (done)

I also added an interdiff file.

Claudis’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +MidCamp2014

88 Test applied and tests cleanly.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/config/tour.tour.language-edit.yml
@@ -0,0 +1,43 @@
+    body: '<p>You cannot change the code of the language on the site, since it is used by the system to keep track of the language.</p>'

+++ b/core/modules/language/config/tour.tour.language.yml
@@ -0,0 +1,43 @@
+    body: '<p>Operations are provided for editing and deleting your languages.</p><p>You can edit the name, code, and directionality of the language.</p><p>Deleted languages can be added back at a later time. Note that you cannot delete the default language of the site.</p>'

Can I change a langcode - yes or no? These statements seem contradictory.

Gábor Hojtsy’s picture

I don't think you can :D

balagan’s picture

I have overlooked that. I also think directionality should be changed to direction.

balagan’s picture

FileSize
9.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,786 pass(es). View

I have deleted "code", and also changed directionality to direciton.

balagan’s picture

Status: Needs work » Needs review
balagan’s picture

Status: Needs review » Needs work

We agreed in another issue that languages are not enabled, but added, although this text says that.

balagan’s picture

FileSize
9.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,863 pass(es). View

Changed "enabled" introduced in this patch to "added". There is still one occurance of enabled in language.module file, but I suppose that might be changed in another issue.

balagan’s picture

Status: Needs work » Needs review
balagan’s picture

Status: Needs review » Needs work
balagan’s picture

FileSize
9.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,983 pass(es). View

Accidentally put a dot instead of a comma. Fixed that.

balagan’s picture

Status: Needs work » Needs review
balagan’s picture

Status: Needs review » Needs work
balagan’s picture

According to this issue languages are not enabled but added.

balagan’s picture

FileSize
9.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,929 pass(es). View

Changed "enabled" to "added" in two yml tour files.

balagan’s picture

Status: Needs work » Needs review
gnuget’s picture

Status: Needs review » Needs work

Hi!

I did a quick review and i noticed one little thing:

+++ b/core/modules/language/language.module
@@ -708,3 +709,41 @@ function language_system_regional_settings_form_submit($form, &$form_state) {
+    }
+    else if ($tour_tip->get('id') == 'language-continue') {
+      $additional_continue = '';
+      $additional_modules = array();
+      if (! Drupal::service('module_handler')->moduleExists('locale')) {
+        $additional_modules[] = 'Interface translation';
+      }

Accord with the coding standards must be "elseif" instead "else if" (https://drupal.org/coding-standards#controlstruct).

Also, i believe this patch needs a reroll.

Regards.

balagan’s picture

Nice catch, but the reason the last patch does not apply is that this part with the "else if" is already committed into core :). I will reroll this patch without this part, and I think a new issue should be opened for this problem.
Lol, I have just applied this patch, that's why I saw it in code. Sorry, I will fix it.

balagan’s picture

FileSize
9.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,751 pass(es), 3 fail(s), and 0 exception(s). View

Here is the rerolled patch.

balagan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 107: drupal8.language-2017471-107.patch, failed testing.

Antti J. Salminen’s picture

Assigned: Unassigned » Antti J. Salminen
Issue tags: +drupalcampfi
FileSize
9.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,740 pass(es). View
Antti J. Salminen’s picture

Status: Needs work » Needs review
Antti J. Salminen’s picture

Assigned: Antti J. Salminen » Unassigned
balagan’s picture

Can you add an interdiff?

Antti J. Salminen’s picture

Sure, hope I created in the correct way.

Might not be all that useful though, actually the only change caused by that issue is that the .yml files had to be moved from config/ to config/install/.

balagan’s picture

As I see there were no changes, only that function language_system_regional_settings_form_submit($form, &$form_state) was moved a few lines up. I really wonder why my patch did not pass the tests then?

Antti J. Salminen’s picture

The linked issue requires moving the configuration to config/install instead of just config. The configuration was not installed when enabling the language module at all without the change. Sorry for not being more clear in the initial posting of the rerolled patch.

Marc Hannaford’s picture

Assigned: Unassigned » Marc Hannaford

Testing in sprint

Kristen Pol’s picture

Issue summary: View changes
Kristen Pol’s picture

Issue summary: View changes
Kristen Pol’s picture

Issue summary: View changes
Kristen Pol’s picture

Issue summary: View changes
Kristen Pol’s picture

Issue summary: View changes
legaudinier’s picture

Assigned: Marc Hannaford » Unassigned
Issue summary: View changes

Went through all the tours and the wording looks clear and easy to follow.

Marc Hannaford’s picture

Reviewed and completed user testing - looks good

Kristen Pol’s picture

Removing old sprint tags.

Kristen Pol’s picture

+++ b/core/modules/language/config/install/tour.tour.language.yml
@@ -0,0 +1,43 @@
+    body: '<p>Operations are provided for editing and deleting your languages.</p><p>You can edit the name, and the direction of the language.</p><p>Deleted languages can be added back at a later time. Note that you cannot delete the default language of the site.</p>'

Thanks for the patch! There is an extraneous comma in here:

"You can edit the name, and the direction of the language"

since "code" was removed.

This is a great easy change for someone to update this patch. :)

And, I'm removing the "Needs tests" tag because tests were added.

Kristen Pol’s picture

Issue tags: -Needs tests

Really removing the "Needs tests" tag this time.

Kristen Pol’s picture

Status: Needs review » Needs work
Marc Hannaford’s picture

Assigned: Unassigned » Marc Hannaford

Trying to do patch

Marc Hannaford’s picture

FileSize
9.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,720 pass(es). View

Removed comma as pointed out in #126.

Marc Hannaford’s picture

FileSize
905 bytes

Adding interdiff file

legaudinier’s picture

Assigned: Marc Hannaford » Unassigned

Tested and the extra comma is gone.

martin107’s picture

Status: Needs work » Needs review

triggering testbot

Antti J. Salminen’s picture

FileSize
9.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,800 pass(es). View

Rerolling, moves tests to the correct directory for PSR-4.

aloyr’s picture

DrupalCon Austin 2014 Sprint - manually testing patch #2017471-134

Suggestion: on the "editing language" tour, step 1 out of 5, add a comma:

From:
This page provides the ability to edit a language on your site including custom languages.

To:
This page provides the ability to edit a language on your site, including custom languages.

aloyr’s picture

FileSize
9.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,799 pass(es). View
718 bytes

uploaded new patch, with suggestion on #135

Nomes’s picture

Issue summary: View changes
tomreavley’s picture

Assigned: Unassigned » tomreavley
tomreavley’s picture

Assigned: tomreavley » Unassigned
mon_franco’s picture

Assigned: Unassigned » mon_franco
mon_franco’s picture

FileSize
9.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,135 pass(es), 1 fail(s), and 0 exception(s). View

The latest patch needed reroll, so please, find attached it. I still working on this issue.

Status: Needs review » Needs work

The last submitted patch, 141: 2017471.135.reroll.patch, failed testing.

penyaskito’s picture

The problem is the edit routing name changed, from language.edit to entity.configurable_language.edit_form
For knowing that, I checked which test failed, in which line, and there I saw that it was looking at language edition path (admin/config/regional/language/edit/en). Then I checked the tour at the patch, and the language.routing.yml in HEAD and found the mismatch.

penyaskito’s picture

Issue tags: +sprint

Adding to the sprint.

mon_franco’s picture

FileSize
552 bytes
9.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2017471.145.patch. Unable to apply patch. See the log in the details link for more information. View

Thanks for your help @penyaskito :)
I have made the change mentioned.
Now I will wait for the new test and once everything goes well, I can start to test the tour help.

penyaskito’s picture

Status: Needs work » Needs review

Nice :-) You need to change the status to Needs Review so the testbot picks the patch.

Status: Needs review » Needs work

The last submitted patch, 145: 2017471.145.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: -sprint

Not being worked on for a month. Removing from sprint.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
552 bytes
10.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,569 pass(es), 2 fail(s), and 0 exception(s). View
jhodgdon’s picture

Great this is being restarted, thanks!

Nitpick on the patch:

+      if (! empty($additional_modules)) {

I don't think we normally have a space between ! and what it is negating in an if()?

Status: Needs review » Needs work

The last submitted patch, 149: 2017471-149.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
36.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,570 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 152: 2017471-152.patch, failed testing.

YesCT’s picture

Assigned: mon_franco » Unassigned
rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
9.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,643 pass(es). View

First the rerol of #150 since #152 is not correctly made.

Let's see what the testbot thinks.

rodrigoaguilera’s picture

FileSize
9.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,697 pass(es). View
3.18 KB

The test in the patch was not running because it had no annotations .

Corrected routing and some selectors.

Now is ready for review.

jhodgdon’s picture

Status: Needs review » Needs work

I read through the tips, and they mostly look good!

A few little things to fix:

a)
"...you will get an additional form where you can provide the name, code and direction of the language."
==> Needs comma after 'code". In the Drupal project, we use serial commas in lists.

b)
"You cannot change the code of the language on the site..."
==> I think I would change "the language" to "a language" here.

c)
"Viewing added languages" [link text]
I think saying "added" here is misleading or kind of incorrect. It should be "Viewing configured languages". This appears in several places in the patch.

d)
"The language name is used throughout the site for all users and is typically written in the same language it represents."
==> I believe this is incorrect? I think you typically need to write the name in English (or the default language of your site), and then translate it into other languages using Config Translation. If you want to provide it in another language, you'd have to set the language of the config to that other language first.
==> I also think the wording here is a bit weird at the end "... language it represents". Hm. Well hopefully this part is going away anyway.

e)
"The "Languages" page provides features for adding, editing,..."
==> I wouldn't say it "provides features for". In hook_help() text, we have typically used language like "allows you to".

f)
"After adding a new language, it will be displayed in the language list and can then be edited, reordered, and deleted."
==> Well. One language can be edited or deleted. The entire list can be reordered. This is imprecise writing.
==> Also this could be more concise: Added languages will be displayed in the language list, and can then be edited and deleted.

g)
"This order will determine the order in which languages are displayed in lists on the site ..."
==> More concise: "The order shown here is the display order for language lists on the site..."

h)
"Note that you cannot delete the default language of the site."
==> Can you delete a language that has been used? For instance if you create some content and translate it into Spanish, can you then delete Spanish? What will happen if you do? I don't know the answers to those questions, but it might be good to figure that out and either say something like "... or languages in use on the site ..." or "... Do not delete languages that are in use..."

i) This tour does not tell me how to set the default language.

j) In the tips_alter function:

"This page also provides an overview of how much of the site\'s interface has been translated for each added language."

That ' should not be escaped within "" quotes.

k) same function

'If the "Interface translation" module is enabled, this page will provide an overview of how much of the site\'s interface has been translated for each added language.'

The module name is Interface Translation (note capitalization), and it should not be in quotes. Thus the whole thing should be in "" and the other ' should not need to be escaped then.

l) The other modules mentioned lower down in that same function need the same capitalization fix... and shouldn't these be in t() calls?

+        $additional_modules[] = 'Interface translation';
+      }
+      if (!Drupal::service('module_handler')->moduleExists('translation_entity')) {
+        $additional_modules[] = 'Content translation';
+      }
...
rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,955 pass(es). View
11.88 KB

Thanks for the thorough review.

I think I took care of all that you noted. I took the information of what happens when you delete a language from the confirmation screen displayed when deleting a language.

jhodgdon’s picture

Status: Needs review » Needs work

Looks mostly great!

Regarding item (d) "If you want to provide the name in another language, you have to set the language of the configuration to that other language first."

So ... sorry for the confusion... I am not even sure this is possible to set in the language config UI. Did you verify that?

Also in the tips alter function:

+        $additional_overview = t("If the Interface Translation module is enabled, this page will provide an overview of how much of the site\'s interface has been translated for each configured language.");

Still do not need "site'\s" with the \ before '

I'm also wondering here:

+      if (!Drupal::service('module_handler')->moduleExists('locale')) {
+        $additional_modules[] = t('Interface Translation');
+      }
+      if (!Drupal::service('module_handler')->moduleExists('translation_entity')) {
+        $additional_modules[] = t('Content Translation');
+      }

Maybe the better thing to do here would be to ask the module handler service for the name of the module, rather than hard-wiring it into this tip? Try ModuleHandler::getName('machine_name'). Sorry I didn't think of that before the last review. :)

The rest looks good... but someone more familiar with i18n should probably try out the Tours and verify that they are all accurate.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
13.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,051 pass(es). View
2.92 KB

Regarding item (d) "If you want to provide the name in another language, you have to set the language of the configuration to that other language first."

So ... sorry for the confusion... I am not even sure this is possible to set in the language config UI. Did you verify that?

No, is not possible in the language config UI, you can only change the name in English.
I'm changing the wording to:

If you want to provide the name in another language, you can use the Configuration Translation interface.

Also translation_entity has been renamed to content_translation

jhodgdon’s picture

Interdiff looks good! I haven't actually tried the tour, and again I'm not sure about the accuracy, but all of the issues I identified have been handled well IMO.

Gábor Hojtsy’s picture

Either the interface translation or config translation modules may be used to translate languages which were in the predefined language list. Languages which were added custom (ie. language code not in the predefined list), they can only be translated with config translation.

rodrigoaguilera’s picture

Ok, I will change the text to reflect that.

jhodgdon’s picture

The text is also about how to *enter* the language name though:

The language name is used throughout the site for all users and is typically written in English and then translated using Configuration Translation. If you want to provide the name in another language, you can use the Configuration Translation interface.

I'm unsure about the "typically", or what happens for someone who doesn't even have English enabled on their site (e.g., installs in Spanish and adds Portuguese).

rodrigoaguilera’s picture

@jhodgdon The name in that interface is entered in English always.

Gábor Hojtsy’s picture

Right now the language entities are all saved in English and then translated from there. Possibly once #2212069: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated lands, they may end up being in some foreign language, but its a tricky question. For now we should assume they are in English by default, that is how its implemented now.

jhodgdon’s picture

Ok then we should take the word "typically" out of the Tour item. ;)

jhodgdon’s picture

So I think this should say:

The language name is used throughout the site for all users and is written in English. Names of built-in languages can be translated using the Interface Translation module, and names of both built-in and custom languages can be translated using the Configuration Translation module.

rodrigoaguilera’s picture

FileSize
13.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,210 pass(es). View
1.03 KB

Changed the tip with that explanation

jhodgdon’s picture

Looks good to me.

I haven't actually tried to run either of the tours, and I'm not an expert on D8MI functionality. So I've reviewed the text for Drupal docs standards, grammar, and clarity, and I think it's good... But someone else should try the tours, verify they are accurate and that they work well (point to the right items on the page etc.), and set to RTBC (or Needs Work if more appropriate). The issue summary lists the pages the tours are on.

balagan’s picture

FileSize
13.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,488 pass(es). View
2.03 KB

Made Content and Interface Translation module names uppercase as noted before, corrected some typo, and I have found "The site will use the default language in situations were no choice is made but a language should be set, for example the language in which the interface of the site is displayed.", which I found a bit confusing. Note this sentence contained the typo too (were instead of where).

rodrigoaguilera’s picture

Thanks, now is more clear indeed

Dom.’s picture

Manual functionnal testing:
- Installed fresh Drupal 8 from pulling latest GIT code + adding patch #171 :
OK, patch installed cleanly. Install done with standard profile install, default english language. Tour is enabled by default, Language is not.
- Enable Language module :
Ok, nothing particular to say.
- Go to: admin/config/regional/language:
Done, only english available and activated.
- Click on tour icon on upper right:
Ok, present directly, no need to clear cache or whatever.
- Walk through first tour :
OK, a popup display centered on screen as expected. English is not my native language so I might not correct any english here but it says "the 'Languages' page". Here are some not directly related notices while going on the Tour :

  1. The page is entitled "Languages" but URL is admin/config/regional/language which could therefore be admin/config/regional/languages ?
  2. No "previous" buttons on Tour.
  3. Instructions where clear enough for my level of english.

- Click "Add language" button
- Click on tour icon on upper right and walk though this Tour

  1. Arguably: I would have called that page "Add new Language" (instead of "Add language") and thus this Tour step 1: "Adding new languages" (instead of "Adding languages")
  2. Step 2 or the this tour at the end : "where you can provide the name, code, and direction of the language." hum... maybe being unfamiliar with this, you might ask yourself what is the direction of a language...

- Go to: admin/config/regional/language, click "edit" on a language:
Done it to edit english.
- Click on tour icon on upper right; walk through third tour:

  1. 1- Maybe "This page provides the ability to edit a language on your site" can be "of your site" instead of "on your site" ? But not sure because of english ^^
  2. 2- If I can't change language code why display it in edit page ?
  3. 2- If I can't change language code why display it in edit page ?

Everything went well here ! Functionnaly speaking it seems to make the deal.

What this test did not include is :

  1. Enable another language and check the Tour in the other language.
  2. Review the patch code itself manually.
  3. Manually run any kind of tests (units tests, PHPSniffer tests, ...)

I do not change to "reviewed by community", mainly because of remaining point 2.

rodrigoaguilera’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the review.

The " not directly related notices while going on the Tour" should be discussed in another issue since they are general D8MI terminology and if they need a change, it should not be in this patch. I think having a previous button would be nice.

The direction of the language is explained in the edit-language tour. I think is enough for not being repetitive.

I find the language code info useful but again, needs opening another issue if you don't want to see it there.

Checking the translation of the tooltips would require to have the strings translated on https://localize.drupal.org or provide a mock translation. I think is not worth it to check the interface translation here and previous tours are not doing it.

There's no logic involved here, just yaml files so no need for unit testing.

I used codessniffer on the lines involved and they are ok.

Overall I thing the patch is good and since it had manual reviews for the patch and for the tour itself I am marking it RTBC. Correct me if tours are not allowed to enter in the beta phase.

rodrigoaguilera’s picture

The previous button is being worked on here
#1921136: Previous button for tour tips

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Adding UI help is not frozen in beta. Committed 35f1560 and pushed to 8.0.x. Thanks!

  • alexpott committed 35f1560 on 8.0.x
    Issue #2017471 by rodrigoaguilera, balagan, clemens.tolboom, Kristen Pol...
rodrigoaguilera’s picture

I'm making other tours and I realize we messed it up with the <p> tags in the body keys of the yaml. They shouldn't be there. How should we proceed to fix this? Another issue?

rodrigoaguilera’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.