Patch fixes

  1. Inconsistency between incorrect classy and correct seven (tablesort-indicator.html.twig)
  2. Fixes incorrect capitalisation of very short words (requires to be ucfirst)

Beta Evaluation

Markup is "unfrozen".

CommentFileSizeAuthor
#28 Issue-2560049-by-hass-Incorrect-capitalisation-of-tr.patch43.49 KBhass
#25 Issue-2560049-by-hass-Incorrect-capitalisation-of-tr.patch42.2 KBhass
#21 Issue-2560049-by-hass-Incorrect-capitalisation-of-tr.patch42.2 KBhass
#13 Issue-2560049-by-hass-chrisfree-Incorrect-capitalisa.patch3.74 KBhass
#10 Drupal-incorrect-capitalization-2560049-10-8.patch4.35 KBchrisfree
#2 Issue-2560049-by-hass-Incorrect-capitalisation-of-so.patch4.01 KBhass

Comments

hass created an issue. See original summary.

hass’s picture

hass commented
Status: Active » Needs review
StatusFileSize
new Issue-2560049-by-hass-Incorrect-capitalisation-of-so.patch4.01 KB

Is there a tag for translation deadline?

hass’s picture

hass commented
Issue summary: View changes
hass’s picture

hass commented
Issue summary: View changes
hass’s picture

hass commented
Issue summary: View changes
hass’s picture

hass commented
Issue summary: View changes
hass’s picture

hass commented
Issue tags: +translatable strings
cilefen’s picture

cilefen commented
Issue summary: View changes
hass’s picture

hass commented
Title: Incorrect capitalisation of source string » Incorrect capitalisation of translatable strings
chrisfree’s picture

chrisfree commented
StatusFileSize
new Drupal-incorrect-capitalization-2560049-10-8.patch4.35 KB

Patch in comment #2 looked good to me, but I found two instances in the documentation for pager.html.twig that also needed to be updated for consistency. The attached patch fixes those as well.

Hat tip to @mortendk for getting me motivated during his DrupalCon Barcelona session to start helping out with Classy!

hass’s picture

hass commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

hass’s picture

hass commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new Issue-2560049-by-hass-chrisfree-Incorrect-capitalisa.patch3.74 KB

Ok, one file has already been fixed in an other case, so just removed this stuff now. Waiting for green light.

hass’s picture

hass commented
Status: Needs review » Reviewed & tested by the community
cilefen’s picture

cilefen commented

#2573437: Paging links are lowercased by default

hass’s picture

hass commented
Status: Reviewed & tested by the community » Needs work

Per https://www.drupal.org/node/2573437#comment-10402251 we also have these strings in Views installation files. Let's fix them with the patch, too.

hass’s picture

hass commented

@crissfree: in what files are these config strings?

chrisfree’s picture

chrisfree commented

Let me dig them up -- give me a few minutes.

chrisfree’s picture

chrisfree commented

There are quite a few places that this would need to be updated. I've included a list of all occurrences of "previous:" within yaml files below for a clearer picture.

A quick example would be the front page view that is installed with the standard site profile. This file can be found at: /core/modules/node/config/optional/views.view.frontpage.yml. The lines in question would be:

tags:
            previous: '‹ previous'
            next: 'next ›'
            first: '« first'
            last: 'last »'

which would become:

tags:
            previous: '‹ Previous'
            next: 'Next ›'
            first: '« First'
            last: 'Last »'

Here are all the instances of "previous: " within yaml files in D8 core:

⚡ ack --yaml previous:
core/modules/aggregator/config/optional/views.view.aggregator_rss_feed.yml
62:            previous: '‹ previous'

core/modules/aggregator/config/optional/views.view.aggregator_sources.yml
66:            previous: '‹ previous'

core/modules/block_content/config/optional/views.view.block_content.yml
55:            previous: '‹ previous'

core/modules/block_content/tests/modules/block_content_test_views/test_views/views.view.test_block_content_view.yml
61:            previous: '‹ previous'

core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_field_name.yml
63:            previous: '‹ previous'

core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_rest.yml
68:            previous: '‹ previous'

core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_row.yml
64:            previous: '‹ previous'

core/modules/contact/tests/modules/contact_test_views/test_views/views.view.test_contact_link.yml
61:            previous: '‹ previous'

core/modules/entity_reference/tests/modules/entity_reference_test_views/test_views/views.view.test_entity_reference_entity_test_mul_view.yml
61:            previous: '‹ previous'

core/modules/entity_reference/tests/modules/entity_reference_test_views/test_views/views.view.test_entity_reference_entity_test_view.yml
61:            previous: '‹ previous'

core/modules/entity_reference/tests/modules/entity_reference_test_views/test_views/views.view.test_entity_reference_reverse_entity_test_mul_view.yml
61:            previous: '‹ previous'

core/modules/entity_reference/tests/modules/entity_reference_test_views/test_views/views.view.test_entity_reference_reverse_entity_test_view.yml
61:            previous: '‹ previous'

core/modules/file/config/optional/views.view.files.yml
55:            previous: '‹ previous'
792:            previous: '‹ previous'

core/modules/node/config/optional/views.view.archive.yml
63:            previous: ‹‹

core/modules/node/config/optional/views.view.content.yml
41:            previous: '‹ previous'

core/modules/node/config/optional/views.view.frontpage.yml
176:            previous: '‹ previous'

core/modules/node/config/optional/views.view.glossary.yml
64:            previous: ‹‹

core/modules/node/tests/modules/node_test_views/test_views/views.view.test_contextual_links.yml
41:            previous: '‹ previous'

core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_tokens.yml
66:            previous: '‹ previous'

core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_view.yml
63:            previous: '‹ previous'

core/modules/taxonomy/config/optional/views.view.taxonomy_term.yml
66:            previous: ‹‹

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.taxonomy_all_terms_test.yml
64:            previous: '‹ previous'

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.taxonomy_default_argument_test.yml
63:            previous: '‹ previous'

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_argument_taxonomy_index_tid_depth.yml
64:            previous: '‹ previous'

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_filter_taxonomy_index_tid.yml
64:            previous: '‹ previous'

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_filter_taxonomy_index_tid_depth.yml
64:            previous: '‹ previous'

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_parent.yml
63:            previous: '‹ previous'

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_term_name.yml
63:            previous: '‹ previous'

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_term_relationship.yml
64:            previous: '‹ previous'

core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_tid_field.yml
63:            previous: '‹ previous'

core/modules/user/config/optional/views.view.user_admin_people.yml
53:            previous: '‹ previous'

core/modules/user/tests/modules/user_test_views/test_views/views.view.test_user_roles_rid.yml
61:            previous: '‹ previous'

core/modules/views/config/schema/views.data_types.schema.yml
609:        previous:

core/modules/views/tests/modules/views_test_config/test_views/views.view.numeric_test.yml
64:            previous: '‹ previous'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_dependency.yml
71:            previous: '‹ previous'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_validator_term.yml
64:            previous: '‹ previous'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_attached_disabled.yml
65:            previous: '‹ previous'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_disabled_display.yml
65:            previous: '‹ previous'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_entity_test_link.yml
61:            previous: '‹ previous'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_entity_test_protected_access.yml
64:            previous: '‹ previous'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_mini_pager.yml
38:            previous: '‹‹ test'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_row_render_cache.yml
63:            previous: '‹ previous'

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_token_view.yml
63:            previous: '‹ previous'

I'll he happy to fix this up and patch this afternoon if this makes sense to move forward with.

hass’s picture

hass commented
Issue tags: +rc deadline
hass’s picture

hass commented
Status: Needs work » Needs review
StatusFileSize
new Issue-2560049-by-hass-Incorrect-capitalisation-of-tr.patch42.2 KB

Thanks chriss for noting this. A search yielded a lot of missing places with bugs. Please RTBC this patch asap. It's simple to review and it is RTBC from my side.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Looks great, with just one remaining problem/inconsistency.

Also: where is it documented that we require the first letter to be capitalized? I didn't realize this was a thing!

  1. +++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
@@ -382,10 +382,10 @@
-      '« first' => '« eerste',
-      '‹ previous' => '‹ vorige',
-      'next ›' => 'volgende ›',
-      'last »' => 'laatste »',
+      '« First' => '« eerste',
+      '‹ Previous' => '‹ vorige',
+      'Next ›' => 'volgende ›',
+      'Last »' => 'laatste »',

    The Dutch translations here remain uncapitalized. 

  2. +++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
@@ -410,10 +410,10 @@
-      '« first' => '« eerste',
-      '‹ previous' => '‹ vorige',
-      'next ›' => 'volgende ›',
-      'last »' => 'laatste »',
+      '« First' => '« Eerste',
+      '‹ Previous' => '‹ Vorige',
+      'Next ›' => 'Volgende ›',
+      'Last »' => 'Laatste »',

    Yet here they became capitalized.

    Which is correct?

hass’s picture

hass commented
Related issues: +#421118: [Meta] Standardize capitalization on actions
hass’s picture

hass commented

I guess the ucfirst is the right one. Damn oversight. The lowercase strings just create the same bugs in nl. We made the same faults in german as we don't know if lowercase is required.

hass’s picture

hass commented
Status: Needs work » Needs review
StatusFileSize
new Issue-2560049-by-hass-Incorrect-capitalisation-of-tr.patch42.2 KB

Fixed the inconsistency.

Status: Needs review » Needs work

The last submitted patch, 25: Issue-2560049-by-hass-Incorrect-capitalisation-of-tr.patch, failed testing.

The last submitted patch, 25: Issue-2560049-by-hass-Incorrect-capitalisation-of-tr.patch, failed testing.

hass’s picture

hass commented
Status: Needs work » Needs review
StatusFileSize
new Issue-2560049-by-hass-Incorrect-capitalisation-of-tr.patch43.49 KB
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

The patch makes the same changes very consistently. #421118: [Meta] Standardize capitalization on actions has all kinds of people looking into it, so it seems like this is indeed a movement agreed on :)

xjm’s picture

xjm
she/her
Primary language English
commented
Component: Classy theme » user interface text
Status: Reviewed & tested by the community » Fixed

Ah, this makes me happy. :) It brings these strings in line with the other actions we made sentence-cased already. Nice work on this issue! Committed and pushed to 8.0.x.

  • xjm committed 176d994 on 8.0.x 
    Issue #2560049 by hass, chrisfree, cilefen, Wim Leers, Gábor Hojtsy:...

Status: Fixed » Closed (fixed)

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