Problem/Motivation
Part of #2924351: Fix coding standards issues with existing settings tray JavaScript. @drpal noticed that there are a number of comments in the JavaScript file that are not correctly wrapped at 80 characters.
Note that normally we would fix one coding standard rule across all of core, as explained in https://www.drupal.org/core/scope#coding-standards, but phpcs.xml.dist
defines PHP standards, not JS standards. We're scoping this to only the JS code and only Settings Tray because it's an experimental module (which means it still goes through a final code review).
Proposed resolution
Update the documentation to be wrapped correctly.
Remaining tasks
- Rewrap the comments in Settings Tray JavaScript files (
core/modules/settings_tray/js/settings_tray.es6.js
). - We don't need to re-transpile it to ES5 because there are no comments in the transpiled files.
Comment | File | Size | Author |
---|---|---|---|
#9 | 2924365.patch | 1.01 KB | ioana apetri |
Comments
Comment #2
ioana apetri CreditAttribution: ioana apetri at OPTASY commentedI wrapped the comments in this module to 80 character limit. Here is my patch. It remains this file SettingsTrayFormAnnotationNoneBlock.php
It is needed to rename the admin label block from "Settings Tray test block: forms[settings_tray] is not specified" to smth shorter to be on a single line.
Comment #3
xjmThanks @yo30! Nice work on this. (Also, welcome to Drupal.org!)
A couple of points of feedback:
This comment line was already less than 80 characters long, so we don't need to rewrap it here.
.es6.js
files inside Settings Tray (not the.php
files). The reason for this is that it came up in a JavaScript maintainer's review of the new JS code in Settings Tray (which we do to mark the module stable). There are a number of other comments longer than 80 characters insettings_tray.es6.js
.Can you also provide an updated patch for that? Thanks again.
Comment #4
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedDid changes only settings.es6.js as per #3.
Comment #5
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #6
ioana apetri CreditAttribution: ioana apetri at OPTASY commentedOk, I reviewed this file now. Thanks for feedback:)
Comment #7
xjmThanks @yo30. Those comments are still wrapping too early. They should go as close as possible to 80 characters without going over.
@gaurav.kapoor's patch is closer to what we need, but I noticed that it doesn't apply on 8.5.x. So let's create a patch for 8.5.x first, and then we can use #4 as a backport.
Thanks!
Comment #8
xjmUpdating the summary to explain why we have this issue scope. :)
Comment #9
ioana apetri CreditAttribution: ioana apetri at OPTASY commentedComment #10
xjmThat looks good now, thanks!
Comment #11
xjmComment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks like in this line there are no violations of length.
Edit: But the next line can be longer
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedopps, my last edit in #12 goes beyond of the scope. Please, ignore it.
Also this patch contains changes only in settings_tray.es6.js. But we need to do it for settings_tray.js too. See https://www.drupal.org/node/2815083
Comment #14
xjm@vaplas, as I mention above, there are no comments in the ES5 files, so the transpiled version is the same as what's in HEAD.
So this is still RTBC.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedIndeed, my bad. Sorry for this. @xjm thank you for clarifying again.
But why we need these changes? There are exactly 80 characters.
Comment #16
xjmAh, so that one character is a gray area. We had a discussion about this several years ago because the interpretation of the 80 characters depends on whether you include the line ending character. Historical 80 character limits were 79 characters plus the line ending character (and that's also the point where my window settings add wrapping), but many IDEs and linting tools don't count the line ending character. So I usually accept patches either way if the line would include exactly 80 visible characters. :)
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commented@xjm++!
I did not even think about
\n
. Before I like it so much when the line fits exactly 80 visible characters. Now the world will not be the same :( Sorry for noise!Comment #18
catchCommitted 3874554 and pushed to 8.5.x. Thanks!
Comment #21
xjmI committed #4 to 8.4.x as a backport as well.