See #2915784: 1/3 JS codestyle: camelcase for more information.

This issue is just the changes to color module, including the BC layers.

CommentFileSizeAuthor
#20 2927228-20.patch6.42 KBmikejw
#16 2927228-16.patch6.53 KBmikejw
#10 interdiff-2927228-7-10.txt420 bytesGrandmaGlassesRopeMan
#10 2927228-10.patch6.29 KBGrandmaGlassesRopeMan
#7 2927228-7.patch6.57 KBGrandmaGlassesRopeMan
#7 interdiff-2927228-6-7.txt254 bytesGrandmaGlassesRopeMan
#6 interdiff-2927228-2-6.txt885 bytesGrandmaGlassesRopeMan
#6 2927228-6.patch6.91 KBGrandmaGlassesRopeMan
#2 2927228-2.patch7.63 KBGrandmaGlassesRopeMan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drpal created an issue. See original summary.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
FileSize
2927228-2.patch7.63 KB
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Looking at the code again the shift_color method is never exposed, as its just defined in the scope of the attach method. Given that I don't think we need a BC layer.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Needs work

I would argue: Let's make it simpler, as its not needed. I would though also argue: Let's not merge it into another issue, as this is a hard problem to think about.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Needs review
FileSize
2927228-6.patch6.91 KB
interdiff-2927228-2-6.txt885 bytes

- Removes the BC layer. Scope is hard, shift_color was only available within the scope of Drupal.behaviors.color.attach.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
FileSize
interdiff-2927228-6-7.txt254 bytes
2927228-7.patch6.57 KB

- returns .eslintrc.passing.json to it's original state.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ b/core/modules/color/color.js
@@ -93,6 +93,10 @@
 
+      function shift_color(given, ref1, ref2) {
+        return shiftColor(given, ref1, ref2);

This looks like you did not ran the build step :(

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

I've fallen victim to my own process.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
FileSize
2927228-10.patch6.29 KB
interdiff-2927228-7-10.txt420 bytes

- Actually includes the transpiled code.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you @drpal!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

I got a bit confused by the issue summary which says this includes colour module fixes + BC layers, but no BC layers. :D I see this was removed per #4. When I asked @dawehner for more info on Slack, he pointed out that the shift_color() function isn't accessible from outside, so therefore the BC layer isn't needed. (See https://jsfiddle.net/dc4jpL26/)

Okie doke then!

Committed and pushed to 8.5.x. Thanks!

  • webchick committed 27253f7 on 8.5.x 
    Issue #2927228 by drpal, dawehner: 2/3 JS codestyle: camelcase

  • xjm committed f7e2d52 on 8.5.x 
    Revert "Issue #2927228 by drpal, dawehner: 2/3 JS codestyle: camelcase...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Fixed » Needs work

Had to revert as mentioned in the other issue; please reroll and this can probably go back in with some quick re-review and testing. Thanks!

mikejw’s picture

mikejw CreditAttribution: mikejw at University of Adelaide commented
FileSize
2927228-16.patch6.53 KB

Here be the reroll. Similar to 1/3 wasn't a straight reroll.

mikejw’s picture

mikejw CreditAttribution: mikejw at University of Adelaide commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you for your patch @mikejw

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

Trying to commit this, but https://github.com/alexpott/d8githooks is giving me:

$ node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/8.x/core/modules/color/preview.es6.js
[09:44:04] '/Users/angie.byron/Sites/8.x/core/modules/color/preview.es6.js' is being checked.
[09:44:05] '/Users/angie.byron/Sites/8.x/core/modules/color/preview.es6.js' is not updated.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn run v0.24.6
$ node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/8.x/core/modules/color/preview.es6.js
[09:44:06] '/Users/angie.byron/Sites/8.x/core/modules/color/preview.es6.js' is being checked.
[09:44:06] '/Users/angie.byron/Sites/8.x/core/modules/color/preview.es6.js' is not updated.
error Command failed with exit code 1.

Any ideas?

mikejw’s picture

mikejw CreditAttribution: mikejw at University of Adelaide commented
FileSize
2927228-20.patch6.42 KB

Yeah, needed to run build:js over it ;)

mikejw’s picture

mikejw CreditAttribution: mikejw at University of Adelaide commented
Status: Reviewed & tested by the community » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Okay, take two! :)

Committed and pushed to 8.5.x. Thanks!

  • webchick committed 01576f2 on 8.5.x 
    Issue #2927228 by drpal, mikejw, dawehner, webchick, xjm: 2/3 JS...

Status: Fixed » Closed (fixed)

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