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.
- I don't **think** davysgrey is a proper noun or anything special that would prevent us from changing it to "davysgray".
- Greyskull is an incorrect spelling of "Castle Grayskull" https://en.wikipedia.org/wiki/Castle_Grayskull
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?
Comment | File | Size | Author |
---|---|---|---|
#10 | interdiff-3155258-7-10.txt | 333 bytes | jwilson3 |
#10 | 3155258-10.patch | 80.29 KB | jwilson3 |
Comments
Comment #2
jwilson3Basic search/replace inside "core" folder.
Comment #3
jwilson3Comment #4
jwilson3Add "grey" to flagWords in .cspell.json
Comment #5
jwilson3The 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.
Comment #6
jwilson3Mentioned impact of UI string in issue summary.
Comment #7
jwilson3The 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.
Comment #8
jwilson3Before patch in #7:
After patch:
Comment #9
alexpottThis list is sorted alphabetically so this is in the wrong place.
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.
Comment #10
jwilson3Thanks @alexpott
-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.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).Comment #11
jwilson3NR for patch and for feedback from @lauriii to address comment #9 point 2 (see also #10 point 3).
Comment #12
lauriii#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.
Comment #13
jwilson3Thanks @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?
Comment #14
longwaveAll cases are fixed by this patch, all review points have been addressed, @lauriii has given his blessing, this is ready to go.
Comment #15
alexpottCommitted 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
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.