Background:
This issue is part of the meta-issue for updating the hook_help texts of the Drupal 8 modules:
#1908570: [meta] Update or create hook_help() texts for D8 core modules
Tasks:
a) [DONE - patch ready] review / write the hook_help text according to help guidelines http://drupal.org/node/632280
More details are on the meta-issue.
b) (novice) Final manual testing:
1. Apply the patch.
2. Go to admin/help.
3. Click on the help page for this module (Language).
4. Verify that the help page is OK:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.
Comment | File | Size | Author |
---|---|---|---|
#42 | language-help-text-2103039-42.patch | 12.12 KB | fran seva |
#42 | interdiff_2103039_41-42.txt | 2.72 KB | fran seva |
#41 | language-help-text-2103039-41.patch | 12.06 KB | fran seva |
#41 | interdiff_2103039_38-41.txt | 2.99 KB | fran seva |
#38 | language-help-text-2103039-38.patch | 12.06 KB | fran seva |
Comments
Comment #1
ifrikworking on this during DrupalDevDays
Comment #2
ifrikThis is an initial re-write of the help text for the language module. Some things still need fixing:
I'm also not quite sure whether I've now covered everything that the language module does, but the text should work as a starting point.
Comment #3
jhodgdonadmin/config/regional/settings seems to be in core/modules/system/system.routing.yml.
Comment #4
ifrikThanks jhodgdon!
I've added the link as well as the use "block visiblity"
Comment #5
jhodgdonHere's a catalog of what the Language module actually does/provides (from a UI perspective):
- Managing languages (add/delete/edit individual languages and an Overview page at admin/config/regional/language -- route language.admin_overview)
- Language detection/selection settings (admin/config/regional/language/detection -- route language.negotiation -- and quite a few sub-pages for configuring individual selection/detection methods)
- Content language settings page (admin/config/regional/content-language -- route language.content_settings_page)
- Language switcher block
So, we should make sure all of these are covered in this help text, and nothing else.
Some other thoughts on this latest patch:
a)
I think we should not say that you are "maintaining a list of languages". That is kind of weak -- and why would anyone care about maintaining a list? What it really allows you to do is configure the languages used on your site.
b) Make sure the pages referred to in the help are the same as the actual page titles. Most have "Sentence case" capitalization, I think, not "Title Case". For instance, I saw "... Languages List page..." in the help, but the page is actually just called "Languages". And then later in that paragraph, you refer to the language being "shown in the Languages List" -- I don't think that needs capitalization at all.
c)
Needs comma after "name".
d)
Tick box is not the right term. I think it is a set of checkboxes?
e)
No "the" needed here.
f)
This should be "entity" not "content element", and I don't think we refer to anything as "user profiles" any more?
g) Be careful about URLs that go into other modules. For instance, the Block module is no longer required, so if you want to include a Block module URL, you have to check whether it is installed (and also you should say in the text something like "... if the Block module is installed..."). There are examples of this in the Menu module help.
h) I don't understand why the help text recommends not changing the default site language.
i)
This is a bit awkward... In English, it is actually preferable to say "which language to display interface text in" rather than putting the "in" first. And the "by which" part would be better as "provides several methods for detecting..."
j) There are some extra spaces here and there, like:
Thanks!
Comment #6
ifrikThanks, I've taken up the issues.
The phrase "content element" came from the help text displayed directly on the Content language page. I think there is quite a lot of help text _on_ the pages, that probably should be checked for phrases like that. The same goes for the page names: There is a bit of inconsistency about whether the page titles are the same as the selected tab or not, and some of them are very generic (for example "Import" or "Settings") so usability starts to become an issue, and should be taken up at another point.
Comment #7
jhodgdonLooking better!
A few notes:
1. Uses #1
The truth of this statement depends on how you have the Interface Translation module configured, at least according to your latest locale help patch (I think you can turn this on/off, right?). So... not sure what to say here...
2. Uses - Content language
of ==> on
3. Same Uses bullet
We have a UI text guideline that says to use the word "configure" rather than "setting". So this header should probably be changed?
https://drupal.org/node/604342
4. Language switcher block uses
This is not going to work. If the Block module is not installed it will cause an error. See my earlier comment -- need to do what is done in the Menu module help code to check if Block is installed.
5. Please get rid of "by which" throughout.
6. Extra spaces: " specifing de for German would result..."
7.
Is this right? It seems like this is "user" setting documentation?
Comment #8
ifrikThanks.
I've also looked at the style guide with Lewis, and in the next days I'll look at the interface texts and page and tab titles wiith people from the Multilingual sprint to see what needs changing.
After that I'll run through these help texts again.
Comment #9
ifrikI've added the checking whether modules exist to all the references where it says "if enabled...", and taken up the comments above.
The "administration language" is a bit weird, but correct like that. Administrators can add a second language for administrating the site - which is a great option for site builders. However, if the detection method is not enabled, then that field is hidden from the user profile. I've added that to the help text.
The name of that method is somewhat weird as well, and the Multilingual Initative would be happy to replace it with something better.
And sorry, the interdiff might be a bit messy, due to issues with pulling Head in between.
Comment #10
jhodgdonThanks, looks good! A few typos and minor fixes:
a) in 'Setting administration languages'
enable => enabled
b) This "administration languages" Uses item seems a bit out of place. It is currently before the "Selection and Detection" stuff is even described, and before any other mention that people can even set a language for themselves. I like the wording, but... maybe it should be moved a bit? Not sure where... Or maybe it just needs a little more explanation?
c) Detection and Selection section:
===>
page provides several methods for deciding which language should be used for displaying interface text.
(does that sound better? It does to me)
d) Same section - You might want to mention that you choose methods and order them, and the first method that provides an answer is selected?
e) Session item in this section:
for => from?
f) same item
within => as?
g) Administration item within that section:
This area doesn't explicitly say that it only applies to administration pages?
Comment #11
ifrikThanks,
I've taken the comments up, removed the additional idents that crept in at the last patch, and fixed some typos.
I've moved the administration page language just above the longer "Detection and selection" section. To keep them closer together.
In other cases, I would just refer to it in the "Detection..." section, but the second language field on the profile page is hidden, when this method is not available - something that's very easy to overlook and I feel that it deserves the extra attention.
Comment #12
batigolixImpressive rewrite!
I reviewed the patch:
- formatting is Okay
I found a couple of small things
a) missing closing
</td>
b)
I would prefer something like:
c)
I would prefer:
d)
I would add a comma for readability:
e)
Should be:
?
f)
I think we can do without "to your site". It makes the sentence easier to read:
g)
this is a bit wonky. how about:
h)
Punctuation seems weird to me, I'd prefer:
This occurs twice.
g) Should we mention "Content language detection" as well here? You only mention interface text translation ...
Comment #13
ifrikThanks,
I've taken the comments up. With the Detection and selection, we need to explain that once an interface language has been selected, the other methods aren't used. I hope it flows better now.
The Content language selection is not covered here because that's provided by the Content translation module (and I forgot we forgot it there...)
Comment #14
jhodgdonThanks! But I think this still needs a bit of work, sorry...
a)
The module is called "Block", not "Blocks" now.
b) I'm still having trouble with this part:
So, this appears before the section on Detection and Selection, and it refers to a "second language", when as far as I can see, the "first language" hasn't been explained.
Maybe we should replace this with "Setting user languages", and explain that users can select maybe a prefered language and maybe an administration language (both of those settings can apparently be turned off?), and that both of them can be used for detection and selection optionally (see below)? [the "see below" would at least clue people in that the explanation of detection and selection is coming up]
c)
(in URL under Detection and selection) --> Empty space before "would" here.
d) In Session detection item:
needs "the" after "as" here.
e) User detection item:
How about omitting the first "user's"?
f) Administration detection item:
administration => administrative
add => configure
second => ... Again, I don't think it makes sense to say "second" here. What does it say in the UI? Use that text?
g) Selected detection item:
Needs 'the' after 'as'
Comment #15
batigolixChanging component to get feedback from maintainers
Comment #16
ifrikGood idea to make a use "Setting user languages".
How about:
Setting user language
Users can set a Site language on their profile page. This language is used for email messages, and can be used by modules to determine a user's language. It can also be used for interface text, if the User method is enabled as a Detection and selection method (see below). Administrative users can set a separate Administration pages language to be used for interface text on administration pages. This configuration is only available on the user's profile page if the Account administration pages method is enabled (see below).
Language detection and selection
...
Comment #17
ifrikTaken up the previous comments, removed white space.
I think the user languages works like that, but if somebody has a better proposal to rephrase that section then I'll be happy to hear it.
Comment #18
ifrikComment #19
ifrikNo changes to the text, but I've also fixed the links for the cases. The interdiff-17-19 only covers these.
The UI text is not changed but will need to be edited at some stage, because it doesn't fit with the style guide and for example refers to "Drupal" instead of to "your site".
Comment #20
jhodgdonUm. The patch in #19 lost most of the patch from #17 (most of the help changes are missing)? And the 13-17 interdiff is for some other patch I think?
I'll wait to review until we have the patches/interdiffs you intended here.
Comment #21
ifrikThat was most certainly messy....
So here is the patch with the both the new text as it was in patch 17 and the changes to the link in patch 19. Since there wasn't any review, I add an interdiff for 13 and 17
Comment #22
jhodgdonOK, that was thorough of you. :)
So I looked through the patch in #21. It looks great!
Do we want to edit the text in the case: section of the hook_help now?
- We shouldn't be using Drupal in there ( case 'admin/config/regional/language/detection/browser')
- Need "the" at the start of this sentence: "Default language can be changed at the Regional settings page." (case 'admin/config/regional/language/detection')
- Last case: "(eg." should be "(for example, " or else "(e.g., "
Didn't see any other problems...
Comment #23
jhodgdonAnyway, this is probably ready for a manual test - updated the issue summary.
Comment #24
Gábor HojtsyTagging with D8MI topic tags.
Comment #25
jhodgdonWe just had a change to hook_help, on this issue: #2183113: Update hook_help signature to use route_name instead of path.
Here is the change record: https://drupal.org/node/2250345
This patch will need a reroll for this change.
Comment #26
fran seva CreditAttribution: fran seva commented@ifrik I'm going to start to work in manual test but first It's needed a reroll. Any problem if I do the reroll for your patch?
Comment #27
jhodgdon@fran seva: I am pretty sure ifrik will not mind. She's been working on a lot of patches for these issues in collaboration with a lot of people. Please do!
Comment #28
fran seva CreditAttribution: fran seva commented@jhodgdon attach reroll patch and interdiff.
In the rerolling, I have merged the @ifrik code with last code.
I'm not sure about the interdiff, I've followed the instruction in https://drupal.org/documentation/git/interdiff
Comment #29
jhodgdonYeah, don't worry about an interdiff... if you are rerolling a patch because some underlying code has changed, you cannot really make an interdiff that is useful.
So... This change at the top of your new patch is not good:
Can you take those two spaces back out please?
You also apparently added an extra lines that is not needed, here:
Other than those two spots, the reroll looks OK. Thanks!
Comment #31
fran seva CreditAttribution: fran seva commented@jhodgdon thanks for your review!!
here is the new patch without wrong spaces but I think the patch will not pass the tests. The new one is the same that the wrong, just for the spaces.
Comment #32
fran seva CreditAttribution: fran seva commentedComment #33
jhodgdonActually, I think the extra spaces before the <?php at the beginning of the file could cause errors. Those spaces would be output at the top of any page that includes the module file, and that is consistent with the test failures on your previous patch. So I think the reroll is correct now. Thanks!
Anyway... Looking at this new patch, I see that between when patch #21 was submitted and now, someone else changed the Language help in this section:
Patch #31 is removing this line:
Patch #21 was removing this line:
And both patches are replacing the removed line with this line:
I think maybe that we should keep the current help that is in Drupal Core for this page, and take this change out of the patch entirely, because it seems like more correct information.
And here were some other comments I made in #22, for the specific-page section of the hook_help:
- We shouldn't be using Drupal in there ( case 'admin/config/regional/language/detection/browser')
- Need "the" at the start of this sentence: "Default language can be changed at the Regional settings page." (case 'admin/config/regional/language/detection')
- Last case: "(eg." should be "(for example, " or else "(e.g., "
Comment #34
fran seva CreditAttribution: fran seva commentedOk @jhodgdon. I thought that @ifrik changes in "case 'language.admin_overview':" were the last one and valid. I will remove the changes related to language.admin_overview in the patch keeping the last committed version.
I'll review the text taking into account your comments in #22.
thanks for review!!
Comment #35
fran seva CreditAttribution: fran seva commented@jhodgdon I have:
- modified the patch to not replace the case 'language.admin_overview'.
- replaced "eg" for "e.g.,"
- added "The" at the beginning of the sentence "Default language can be changed at the..."
But I'm not sure about the use of Drupal. Should I replace Drupal for another word as System or remove it? or Would be better change a little the sentence, for example, instead of "Define how to decide which language is used to display page elements (primarily text provided by Drupal and modules" change it for, "Define how to decide which language is used to display page elements (primarily text provided and modules".
Comment #36
jhodgdonLet's see. The text says ... "(primarily text provided by Drupal and modules,..." -- here it should probably just say "primarily text provided by modules". I think that is what you meant?
Comment #37
fran seva CreditAttribution: fran seva commented@jhodgdon, yepa. That was what I meant.
I'll change the texts.
Thanks!!
Comment #38
fran seva CreditAttribution: fran seva commentedChanges and interdiff:
changes:
- modified the patch to not replace the case 'language.admin_overview'.
- replaced "eg" for "e.g.,"
- added "The" at the beginning of the sentence "Default language can be changed at the..."
- Remove Drupal word from texts.
Comment #39
jhodgdonI think this patch is looking really really good!
I read through the whole thing, and I think it could use just this one small change. In the Uses section about detection and selection:
I think it should say "... a domain needs to be specified..." here, not a URL?
Also, this patch still needs a manual test. See issue summary for instructions.
Comment #40
fran seva CreditAttribution: fran seva commented@jhodgdon I'll change it and I'll start with the manual test. In the last D8MI meeting @GaborHojtsy proposed me work in this issues and run the manual test :)
Thanks for review!!!
Comment #41
fran seva CreditAttribution: fran seva commentedpatch and interdiff, changing URL for domain.
Next step manual testing.
Comment #42
fran seva CreditAttribution: fran seva commentedManual test review:
1. Apply the patch:
Patch applied language-help-text-2103039-41.patch #41
(Active All multilingual modules)
2. Go to admin/help. OK
3. Click on the help page for this module (Language). OK
4. Verify that the help page is OK:
- Verify that all the links work : At this step the test fails. The link to Detection and selection redirect to Page not found.
Reviewing the code I see that the t() doesn't have the placeholder for !detection. I have change it and create a patch (attached in the comment) in which the link is working.
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
I see that all mentions of pages/text/buttons are corrects. I have seen that in Adding custom languages text section, Languages is being reference but there is no link to Languages. IMHO, as a supposed new user reading the help text I expect a link where I could add the language and check in the drop down list the Custom language option.
- Verify that the formatting is
I don't see errors.
Comment #43
rodrigoaguileraComment #44
rodrigoaguileraI did all the verifications and checked that the last change from fran seva is ok.
I think that the link to Languages from the Adding Languages section is enough.
Comment #45
jhodgdonThanks for the updates and testing! We are in "critical and major commits only" period until after Wednesday when the next Alpha is released. So, this can get committed on Thursday or later.
Comment #46
fran seva CreditAttribution: fran seva commentedI think this issue should wait until we get feedback about what to do with "Drupal translation server" sentence [1]
[1] https://drupal.org/node/1861930
Comment #48
fran seva CreditAttribution: fran seva commentedI'm on it
Comment #49
fran seva CreditAttribution: fran seva commented42: language-help-text-2103039-42.patch queued for re-testing.
Comment #50
fran seva CreditAttribution: fran seva commentedAfter retest the patch, it's green. Change the state to needs work, we are waiting feedback from #1861930: Use "Drupal translations website" instead of Drupal translation server or Community translation server
Comment #51
Gábor HojtsyNot sure we should block this on that. That issue looks like going nowhere. We can fix this here and still amend it with Drupal or without Drupal mentioned on that issue.
Comment #52
bendev CreditAttribution: bendev commentedI have applied #42 - I read the texts It seems clear to me.
As a new D8 user , I could apply the indications given here to translate a content type but I didn't succeed to translate menu links.
This text didn't help me to find a solution. (but I am not sure if this is because I am still missing something or because of another issue)
I chose "current interface language" for the main navigation menu and checked
"Show language selector on create and edit pages"
If I check "Enable translation" and save it, this option appears unchecked after page reload (no idea if this is normal)
when I edit a menu link I see the dropdown to select a language...
Comment #53
jhodgdonAgreed with Gabor, let's not wait on the decision on that other issue about "Drupal translation server".
The comments in #52 seem to be a separate issue about whether the translation UI works, not about this patch. Please file a separate issue.
So I think we are back to RTBC here, as this patch has been tested and verified.
Thanks!
Comment #54
jhodgdonThanks again everyone! Committed to 8.x.
Comment #56
Gábor HojtsyYay, thanks all!
Comment #57
fran seva CreditAttribution: fran seva commentedyeah!! 1 of 4 hook_help issues fixed :)