Problem/Motivation

Follow-up to #3154539: Implement new Gray scale on Claro and #3138718: Convert British English spellings to American English, for the umpteenth time to change all instances of "grey" to "gray".

There is an instance of "davysgrey" and "greyskull" in the dictionary introduced in #2972224: Add .cspell.json to automate spellchecking in Drupal core I suppose these were added as a way to avoid breaking the new spellcheck feature.

Interestingly enough, CSS supports both spellings, but seeing a mixed American english spelling and British English string like: color: grey; is funny.

Proposed resolution

Perform a search/replace of all strings. Add "grey" to flagWords in .cspell.json

Remaining tasks

User interface changes

There is one UI string from the Shortcuts module that is impacted by this change.
The color name in the Claro theme is changed from --color-davysgrey to --color-davysgray

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jwilson3 created an issue. See original summary.

jwilson3’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
72.91 KB

Basic search/replace inside "core" folder.

jwilson3’s picture

Issue summary: View changes
jwilson3’s picture

FileSize
73.21 KB
304 bytes

Add "grey" to flagWords in .cspell.json

jwilson3’s picture

+++ b/core/modules/shortcut/shortcut.module
@@ -28,7 +28,7 @@ function shortcut_help($route_name, RouteMatchInterface $route_match) {
-      $output .= '<dd>' . t('The Shortcut module creates an add/remove link for each page on your site; the link lets you add or remove the current page from the currently-enabled set of shortcuts (if your theme displays it and you have permission to edit your shortcut set). The core Seven administration theme displays this link next to the page title, as a grey or yellow star. If you click on the grey star, you will add that page to your preferred set of shortcuts. If the page is already part of your shortcut set, the link will be a yellow star, and will allow you to remove the current page from your shortcut set.') . '</dd>';
+      $output .= '<dd>' . t('The Shortcut module creates an add/remove link for each page on your site; the link lets you add or remove the current page from the currently-enabled set of shortcuts (if your theme displays it and you have permission to edit your shortcut set). The core Seven administration theme displays this link next to the page title, as a gray or yellow star. If you click on the gray star, you will add that page to your preferred set of shortcuts. If the page is already part of your shortcut set, the link will be a yellow star, and will allow you to remove the current page from your shortcut set.') . '</dd>';

The one instance of grey in an actual string that is not related to CSS, is this hunk from the Shortcuts module, which is long to read and compare but the part in question talks about the "grey star" versus the "yellow star".

There is specific reference to the core Seven administration theme there, but in the Claro theme the disabled star is hollow/outlined and in that sense only slightly gray. So, I don't know if it makes sense to basically rewrite this string entirely to phase out the mention of Seven, but if so, that should be a follow-up issue with no real impact here.

jwilson3’s picture

Issue summary: View changes

Mentioned impact of UI string in issue summary.

jwilson3’s picture

FileSize
80.27 KB
7.07 KB

The search/replace in #2 was case sensitive so I missed the capitalized instances of Greyskull. After fixing that in the patch here, I've also done a case insensitive search to ensure there weren't any others I missed.

jwilson3’s picture

Before patch in #7:

$ git fetch --all --prune
$ git checkout 9.1.x
$ git log --graph --oneline
* 932b36e4de (HEAD -> 9.1.x, origin/9.1.x)
$ cd core
$ find . -path "*/node_modules/*" -o -path "*/assets/vendor*" -o -type f -print0 | xargs -0 grep -I -i -o -n gray | wc -l
     164

jameswilson@Makak ~/App/Drupal/drupal-9.x-dev/core (9.1.x=) 
$ find . -path "*/node_modules/*" -o -path "*/assets/vendor*" -o -type f -print0 | xargs -0 grep -I -i -o -n grey | wc -l
     104

After patch:

$ find . -path "*/node_modules/*" -o -path "*/assets/vendor*" -o -type f -print0 | xargs -0 grep -I -i -o -n grey | wc -l
       1

$ find . -path "*/node_modules/*" -o -path "*/assets/vendor*" -o -type f -print0 | xargs -0 grep -I -i -o -n grey
./.cspell.json:37:      "grey",

$ find . -path "*/node_modules/*" -o -path "*/assets/vendor*" -o -type f -print0 | xargs -0 grep -I -i -o -n gray | wc -l
     268
alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/misc/cspell/dictionary.txt
    @@ -754,7 +754,7 @@ grandgrandchild's
    -greyskull
    +grayskull
    

    This list is sorted alphabetically so this is in the wrong place.

  2. +++ b/core/themes/claro/css/base/variables.pcss.css
    @@ -14,7 +14,7 @@
    -  --color-davysgrey: #545560;
    +  --color-davysgray: #545560;
    

    So one big question is - is this API. I think this change is fine at this point because claro is still not stable. But I'm going to ping @lauriii and ask.

jwilson3’s picture

Issue summary: View changes
FileSize
80.29 KB
333 bytes

Thanks @alexpott

  1. I edited my comment #8 to fix the grep command to use -o option because the items found were not adding up correctly because there were several multiple instances found within a single line of code-- now they do.
  2. Re #9 point 1: Fixed in current patch. Thank you for catching this. You're amazing.
    $ sort misc/cspell/dictionary.txt > misc/cspell/dictionary.sorted.txt 
    $ diff misc/cspell/dictionary.txt misc/cspell/dictionary.sorted.txt 
    --- misc/cspell/dictionary.txt	2020-06-27 08:18:56.000000000 -0500
    +++ misc/cspell/dictionary.sorted.txt	2020-06-27 08:20:19.000000000 -0500
    @@ -753,8 +753,8 @@
     grandgrandchild's
     granularities
     grayblue
    -greeking
     grayskull
    +greeking
     gripsmall
     groupable
     groupby
    
  3. Re #9 point 2: I brought up the standardization question on #3154539: Implement new Gray scale on Claro where I first noticed this issue, and am proposing to remove the named colors for the neutral gray scale and switch to a decimal based color name variables. I would assume a naming correction would be okay too at this point, but will be sure to note that as an API change in the issue summary. Thanks for the ping to @lauriii.
  4. While running yarn run spellcheck:core to sanity check things here, I found two words that are being flagged as unknown. I assume needs a follow-up issue, but I've asked about this on #2972224-170: Add .cspell.json to automate spellchecking in Drupal core (apologies for crosspost).
jwilson3’s picture

Status: Needs work » Needs review

NR for patch and for feedback from @lauriii to address comment #9 point 2 (see also #10 point 3).

lauriii’s picture

#9.4: I think CSS variables technically are an API. However, as you mentioned, Claro is still experimental. Even if Claro was stable, it probably wouldn't be bound by these restrictions because we consider it as internal. So from BC perspective, I think it's fine to make this change.

jwilson3’s picture

Thanks @lauriii.

AFAIK this issue is ready to go in. I've fixed the one change requested by @alexpott from #9 in my patch on #10 but my understanding is that I cannot RTBC my own work, in this case would that help this show up on core maintainers radars more quickly?

longwave’s picture

Status: Needs review » Reviewed & tested by the community

All cases are fixed by this patch, all review points have been addressed, @lauriii has given his blessing, this is ready to go.

alexpott’s picture

Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed e72fe02 and pushed to 9.1.x. Thanks!
Committed 6b971ce and pushed to 9.0.x. Thanks!

Backported everything but the cspell changes to 9.0.x to keep things aligned and because Claro is experimental.

So when I ran this with the commit tools I got

$ cspell .cspell.json
/Users/alex/dev/drupal/core/.cspell.json:37:8 - Unknown word (grey)

lol it's because it doesn't ignore the config file when you pass the config file in... ie. like yarn run spellcheck .cspell.json - I think we should add .cspell.json to the list of ignored files and be done. So I did that on commit.

diff --git a/core/.cspell.json b/core/.cspell.json
index 3734c19be9..e833de483d 100644
--- a/core/.cspell.json
+++ b/core/.cspell.json
@@ -3,6 +3,7 @@
     "language": "en-US",
     "allowCompoundWords": false,
     "ignorePaths": [
+      ".cspell.json",
       "composer.lock",
       "assets/vendor/**",
       "lib/Drupal/Component/Diff/**",

  • alexpott committed e72fe02 on 9.1.x
    Issue #3155258 by jwilson3, alexpott, lauriii: Use American English...

  • alexpott committed 6b971ce on 9.0.x
    Issue #3155258 by jwilson3, alexpott, lauriii: Use American English...

Status: Fixed » Closed (fixed)

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