Follow-up to #2757749-6: Remove @file tag docblock

phpcbf --standard=Drupal refreshless/

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naveenvalecha created an issue. See original summary.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/js/refreshless.js
    @@ -1,3 +1,7 @@
    +/**
    + * @file
    + */
    
    +++ b/src/RefreshlessPageState.php
    @@ -8,6 +8,9 @@ use Drupal\Core\Cache\Context\CacheContextsManager;
    +/**
    + *
    + */
    
    +++ b/src/Render/MainContent/RefreshlessRenderer.php
    @@ -37,6 +36,9 @@ class RefreshlessRenderer implements MainContentRendererInterface {
    +  /**
    +   *
    +   */
    

    These need work.

  2. +++ b/js/refreshless.js
    @@ -466,7 +472,7 @@
         // librariesBefore: before response is applied
         // response: the response to apply
    -    // drupalSettings: after the response is applied
    +    // drupalSettings: after the response is applied.
    

    This is just making a stubbed comment more inconsistent.

  3. +++ b/src/RefreshlessPageState.php
    @@ -84,7 +87,7 @@ class RefreshlessPageState {
    -   *   The current request, to check whether
    +   *   The current request, to check whether.
    

    This sentence was incomplete. My bad, of course! Just putting a period there doesn't help :)

Wim Leers’s picture

I forgot to say: thanks! :)

naveenvalecha’s picture

Issue tags: +Novice

I forgot to say: thanks! :)

<3 ;)
Adding Novice tag

minakshiPh’s picture

Assigned: Unassigned » minakshiPh
minakshiPh’s picture

Assigned: minakshiPh » Unassigned
Status: Needs work » Needs review
FileSize
9.25 KB
5.74 KB

Added the patch with fix as mentioned in #2

Kindly review.
Thanks!

Venkatesh Rajan.J’s picture

Assigned: Unassigned » Venkatesh Rajan.J
Venkatesh Rajan.J’s picture

Status: Needs review » Reviewed & tested by the community

The changes contained in the patch looks good to me.

Venkatesh Rajan.J’s picture

Venkatesh Rajan.J’s picture

Assigned: Venkatesh Rajan.J » Unassigned
Rajeshreeputra’s picture

anoopsingh92’s picture

+1 RTBC

Rajeshreeputra’s picture

Status: Reviewed & tested by the community » Needs work

below lines from patch doesn't make sense to me specially the special character(…)).

-   *   The current request, to check whether
+   *   The current request, to check whether this region (or block or …) has
+   *   changed.

and

*   The cacheability of a region (or block or …), to check whether it would
    *   have changed compared to the previous page.
    * @param \Symfony\Component\HttpFoundation\Request $request
-   *   The current request, to check whether
+   *   The current request, to check whether.

can we have MR for the same.

anoopsingh92’s picture

Status: Needs work » Needs review

Made the changes and remove the special character(…). Please review and merge the MR.
Thank you

irfan.gul’s picture

Status: Needs review » Reviewed & tested by the community

The changes in the patch looks good to me.

Rajeshreeputra’s picture

Status: Reviewed & tested by the community » Needs work

the patch provided in #6 and MR is different.

rishu_kumar’s picture

I've created a patch.

rishu_kumar’s picture

Status: Needs work » Needs review

tagging my organization.

anoopsingh92’s picture

Hi everyone, MR is opened, Please update the MR. you can commit and push the changes in the ticket branch.
Thank you

Ambient.Impact’s picture

This looks great at first glance - especially the proper dependency injection. I'll try and find the time in the next few days to give it a test run locally. Hiding the patches in favour of the issue fork.

Ambient.Impact’s picture

So I was thinking to myself that I never really was a fan of uppercase NULL, TRUE, or FALSE in either JavaScript or PHP, but didn't have the time right now to get into it, and figured the JavaScript was going to get totally rewritten for the Symfony UX Turbo port. Then I tried this and was reminded that uppercase NULL, TRUE, and FALSE in JavaScript are treated as variables or constants unlike in PHP, so uppercasing them results in errors because they no longer refer to null or boolean values, since JavaScript is a case sensitive language that way.

Anyways, after reverting that, the JavaScript mostly works again, though there's some weirdness with updating URLs in the location bar but I haven't looked at RefreshLess in half a year so it's possible that's related to changes in Drupal core or something else.

I'm a bit confused how the coding standards for PHP (I assume) got applied to a JavaScript file?

roberttabigue’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
64.61 KB

Hi,

I reviewed the changes and confirmed the latest MR was applied cleanly.

Checking patch refreshless.services.yml...
Checking patch src/Ajax/RefreshlessUpdateHtmlHeadCommand.php...
Checking patch src/RefreshlessPageState.php...
Checking patch src/Render/MainContent/RefreshlessRenderer.php...
Applied patch refreshless.services.yml cleanly.
Applied patch src/Ajax/RefreshlessUpdateHtmlHeadCommand.php cleanly.
Applied patch src/RefreshlessPageState.php cleanly.
Applied patch src/Render/MainContent/RefreshlessRenderer.php cleanly.

And all PHPCS errors have been fixed.

I ran this command:
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml


Applied to Refreshless module with 8.x-1.x-dev version and with the Drupal core version of 9.5.x.

Attaching a screenshot and moving this to RTBC,

Thanks.

  • 44e9666c committed on 2.x
    Issue #2808811: Fix coding style to meet Drupal standards.
    

  • minakshiPh authored 9b550c23 on 2.x
    Issue #2808811 by minakshiPh, naveenvalecha: Fix coding style to meet...
Ambient.Impact’s picture

Version: 8.x-1.x-dev » 2.x-dev
Status: Reviewed & tested by the community » Fixed

Merged to 2.x. Thanks everyone!

  • 44e9666c committed on 8.x-1.x
    Issue #2808811: Fix coding style to meet Drupal standards.
    

  • minakshiPh authored 9b550c23 on 8.x-1.x
    Issue #2808811 by minakshiPh, naveenvalecha: Fix coding style to meet...

Status: Fixed » Closed (fixed)

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