There are minor coding standards violations that should be addressed before Settings Tray is marked as stable. This fixes them. 👏🤷‍♀️

These coding standards violations would usually be done in general patch instead of per file but since this currently experimental code it seems logic to fix these before marking the code as stable. @see #2922603: Mark Settings Tray module as stable

CommentFileSizeAuthor
#5 interdiff-2924351-2-5.txt5.06 KBGrandmaGlassesRopeMan
#5 2924351-5.patch3.18 KBGrandmaGlassesRopeMan
#2 2924351-2.patch1022 bytesGrandmaGlassesRopeMan
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
2924351-2.patch1022 bytes

There are no actual changes to the transpiled code. ✌️

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs review » Needs work 
+++ b/core/modules/settings_tray/js/settings_tray.es6.js
@@ -222,7 +222,8 @@
-   * Toggle the js-settings-tray-edit-mode class on items that we want to disable while in edit mode.
+   * Toggle the js-settings-tray-edit-mode class on items that we want to
+   * disable while in edit mode.

I will create a separate issue to re-wrap the comments in the file (since there are more). Meanwhile we can remove this hunk from the patch.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

#2924365: Wrap JS comments in Settings Tray to 80 character limit is in now so this can be rerolled on top of that.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Needs review
FileSize
2924351-5.patch3.18 KB
interdiff-2924351-2-5.txt5.06 KB

- removes 80 character wrapping adjustments
- fixes multiline comment format
- removes use of in operator

Status: Needs review » Needs work

The last submitted patch, 5: 2924351-5.patch, failed testing. View results

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

In general I'm a bit confused ... the entire point of automatic code style checking using our special eslint file is not have these special fix patches,
but now we are okay with having a special case? Maybe we should update the issue summary why this is the way to go.

+++ b/core/modules/settings_tray/js/settings_tray.es6.js
@@ -154,14 +155,18 @@
         // Check to make sure existing dialogOptions aren't overridden.
-        if (!('dialogOptions' in instance.options.data)) {
+        if (!instance.options.data.hasOwnProperty('dialogOptions')) {

I am a bit confused about the use of hasOwnProperty given that https://eslint.org/docs/rules/no-prototype-builtins exists

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Needs review

@dawehner Yes. I agree this is a special case. Perhaps we can update the issue summary to reflect that? Additionally, we've set "no-prototype-builtins": [0] in our existing eslint file. I think once we are done with the coding standards cleanup, we can remove this, however currently we are not doing anything with Set or Map.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Update summary re #7 and #8

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2924351-5.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Reviewed & tested by the community

I don't actually see the new test failure. The last #5 failed was 11/20 and it just passed yesterday 12/13

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2924351-5.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Reviewed & tested by the community

Setting back to RTBC because DrupalCI memory problem

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2924351-5.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Reviewed & tested by the community

Setting back to RTBC because DrupalCI memory problem

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2924351-5.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2924351-5.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Reviewed & tested by the community
webchick’s picture

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

Great, let's get this sucker in!

Committed and pushed to 8.5.x. Thanks!

  • webchick committed 35de151 on 8.5.x 
    Issue #2924351 by drpal, tedbow, xjm, dawehner: Fix coding standards...

Status: Fixed » Closed (fixed)

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