Modernize markup for the locale module

.text class? Seriosly?

I have to admit, I was responsible for this. But ... I did not invent this markup. Both the markup and the accompanied JavaScript are based on the Modules page.

What does .update-description-prefix do?

This class is used in the JavaScript (locale.admin.js) that toggles the visibility of update details. This class was also derived from the class used on the Module page.

If we find a solution here, we may apply that to the Module page too.

Related:
#1987410: [meta] system.module - Convert theme_ functions to Twig converts theme_system_modules_details to twig template. That is the function which outputs the module details on the module page.

Here's a patch for the proposed markup. The only difference is that it uses 'visually-hidden' instead of 'element-invisible'.

#4: locale-markup-1982230-4.patch queued for re-testing.

reroll.

Reroll.

There are selectors in the CSS file such as:

#locale-translation-status-form .expanded .description .text {
  -webkit-hyphens: auto;
  -moz-hyphens: auto;
  hyphens: auto;
}

That need updating.

Cilifen's patch in #9 is a good step forward. Please proceed with update of the CSS selectors.

This markup is an improvement, I don't think it's totally inline with our CSS standards though. I'll find more time to go over the markup and what it's doing and try and make suggetions.

1. +++ b/core/modules/locale/locale.admin.js
   @@ -60,7 +60,7 @@
   +          $tr.find('.update-prefix').text(function () {
   
   +++ b/core/modules/locale/templates/locale-translation-update-info.html.twig
   @@ -18,15 +18,15 @@
   +  <span class="update-prefix visually-hidden">Show description</span>
   

   This can be .update__prefix

2. +++ b/core/modules/locale/templates/locale-translation-update-info.html.twig
   @@ -18,15 +18,15 @@
   +<div class="update-wrapper" tabindex="0" role="button">
   

   .update__wrapper

3. +++ b/core/modules/locale/templates/locale-translation-update-info.html.twig
   @@ -18,15 +18,15 @@
   +    <span class="update-message">{% trans %}Updates for: {{ module_list }}{% endtrans %}</span>
   ...
   +    <span class="update-message">{{ missing_updates_status }}</span>
   

   update__message

4. +++ b/core/modules/locale/templates/locale-translation-update-info.html.twig
   @@ -18,15 +18,15 @@
   +    <div class="update-details">{{ details }}</div>
   

   update__details

First, a reroll.

This incorporates the class name changes from #15. I see that I missed a class name on the reroll, but it is fixed in this patch.

Working on this.

Edited CSS to match the changes made to template and JS previously.

+++ b/core/modules/locale/css/locale.admin.css
@@ -1,4 +1,4 @@
-.locale-translate-filter-form .details-wrapper {
+.locale-translate-filter-form .update__wrapper {

@@ -76,12 +76,12 @@
-#locale-translation-status-form .expand .inner {
+#locale-translation-status-form .expand .update__wrapper {
...
-#locale-translation-status-form .expanded .expand .inner {
+#locale-translation-status-form .expanded .expand .update__wrapper {

Thanks this is looking better. I would like to find out if we can remove the other parts of the selector so we can make it a lot shorter and weaker?

I dropped some classes from the selectors and the "expand" class from the markup as nothing seemed to target it. On the CSS, .expanded I don't think we can drop (because of JS interaction) and I'm also somewhat uncomfortable dropping the form id from the selector... But will listen if someone wants to talk me into it!

+++ b/core/modules/locale/css/locale.admin.css
@@ -76,12 +76,12 @@
+#locale-translation-status-form .update__wrapper {
...
+#locale-translation-status-form .expanded .update__wrapper {

@@ -93,33 +93,33 @@
+#locale-translation-status-form .update__wrapper {
...
+#locale-translation-status-form .expanded .update__wrapper {
...
+#locale-translation-status-form .expanded .update__message {
...
+.js #locale-translation-status-form .update__wrapper {
...
+#locale-translation-status-form .expanded .update__wrapper {
...
+#locale-translation-status-form .update__details {

Thanks. This is looking a lot better. We just need to get rid of that ID. In an ideal world we could only have one class, what if we change the '.update' part to '.locale-translation-update' so we would have classes like '.locale-translation-update__wrapper' etc?

Now with compacted selectors.

We need someone to manual test the patch just to make sure we haven't introduced visual regressions.

+++ b/core/modules/locale/css/locale.admin.css
@@ -1,4 +1,4 @@
-.locale-translate-filter-form .details-wrapper {
+.locale-translation-update__wrapper {

@@ -76,12 +76,12 @@
-#locale-translation-status-form .expand .inner {
+.locale-translation-update__wrapper {

I just noticed that these selectors used to be different and now they are the same. Is this ok? If it is we should merge them

@LewisNyman no that belongs to an another form. Reverted that line.

Changing to Needs work for the manual testing.

@holist In that case can we add a better class to that element as well?

I did some manual testing with the patch at #26 and I did not see any regression or strange behaviour.

Setting to reviewed, removing the manual testing tag.

@LewisNyman, I now looked into the ".locale-translate-filter-form .details-wrapper" part of this dives straight into form API, I don't really think we want to go there in this issue. (I was too caught up in the work I actually get paid for the past month so I have neglected this...)

So maybe this one is as far as it gets for now?

@holist That sounds like a good idea. There are a few frontend issue that get caught up on one, small, really hard to change, part of the mark up and they never make it in.

Let's create a follow up for that small part.

RTBC++

@vermario if you do manual testing can you please add screenshots.

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary. This is a markup change so it is allowed but it is good to get this documented on the issue - so I don't have to this when it comes to commit time.

#32 mentions a followup - could this be created too? Thanks.

I added the beta evaluation table.

@LewisNyman I fear it is not going to be a small issue to change what classes Form API creates?

Can someone help with the screenshots?

This is how I went about preparing my drupal environment:
- Standard installation running on Ubuntu server
- Installed the patch, and enabled German as an additional language
- Used Chromium browser on Ubuntu and took screenshots

Not sure exactly what you're looking for, but hopefully these screenshots will be of some use.
This is my 3rd contribution to Drupal core :-)

@andythomnz: thanks for the screenshots, the relevant ones are the last two, but we would need some modules to update translations for (grab a few contributed modules, so you see something here). The rest belong to pages that are not changed.

Reviewed the code also:

+++ b/core/modules/locale/locale.admin.js
@@ -60,7 +60,7 @@
+          $tr.find('.update__prefix').text(function () {

+++ b/core/modules/locale/templates/locale-translation-update-info.html.twig
@@ -18,15 +18,15 @@
+  <span class="locale-translation-update__prefix visually-hidden">Show description</span>

You did not actually modify anything to have an "update__prefix" class, did you?

I also noticed you removed the 'expand' class. I sense that may have been there to support the expand/collapse feature. Does it work without that? (To verify that it is important to screenshot this as explained at the start of my comment above).

@Gábor Hojtsy, good catch on the "update_prefix", it should be ".locale-translation-update__prefix" indeed. I will post a new version of the patch ASAP.

On the "expand" class, I did not notice any change in functionality when trying out the patch and I did review the code if it was used and didn't find any references. The functionality seems to be attached to the class "expanded".

Thanks Gábor for your guidance, I will attempt to re-roll the patch shortly....

@Gábor Hojtsy,
I got a couple of modules to update translations for, and have re-done the screenshots (attached) for the last 2 pages. Also, there are 2 screenshots of each page, one showing it expanded, and the other collapsed. I have not removed the 'expand' class, however the screenshots evidence that it still works.

As for the "update__prefix" class, I have changed the name of the class in the file /core/modules/locale/locale.admin.js to ".locale-translation-update__prefix" so that the class names in the file /core/modules/locale/templates/locale-translation-update-info.html.twig remain conventional.

Makes sense for me and was in part reviewed by @LewisNyman before. So let's get it :) BTW the escaped strings are to be resolved in #2319233: Double escaped string on Available translation update page once committed :)

Great work everyone, maybe we can create an issue to look over the CSS as a child of #1995272: [Meta] Refactor module CSS files inline with our CSS standards?

Committed 978116e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the summary.

Superb, thanks all!