Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sumitmadan created an issue. See original summary.

sumitmadan’s picture

Assigned: sumitmadan » Unassigned
Status: Active » Needs review
FileSize
24.13 KB

Updated code according to latest drupal.

echoz’s picture

Status: Needs review » Reviewed & tested by the community

Patched this on Drupal 8.0.2, seems to work great, thanks! Note the .info.yml is unchanged.
For those testing this module, installing 8.x-1.0-beta1 without patching, would not show the config screen, or it's menu item.

Dom.’s picture

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

This patch is great. It does have some little issues through.
- some listed below
- hook_permissions() does not exist anymore and should be *.permissions.yml file instead.
- Indentation should be spaces instead of tabs.

Here is a new patch attached listing that. It surely have some other points to be corrected too, I made this very quickly!

  1. +++ b/back_to_top.libraries.yml
    @@ -0,0 +1,35 @@
    +back_to_top:
    

    I would have call it back_to_top_admin maybe since this is about assets for admin page only.

  2. +++ b/back_to_top.module
    @@ -46,133 +24,83 @@ function back_to_top_page_build(&$page) {
    +  $attachments['#attached']['library'][] = 'core/jquery.ui.core';
    +  $attachments['#attached']['library'][] = 'core/jquery.ui.effects.core';
    

    Why incluing this here ? Is it because of back_to_top_js requiring it ? Then it has to be a dependency of the library in *.libraries.yml thus.

  3. +++ /dev/null
    @@ -1,181 +0,0 @@
    -class BackToTopSettingsForm extends ConfigFormBase {
    

    The whole file here is poorly indented

echoz’s picture

#4 patch applied cleanly and works fine. The coding standards fixes seem certainly an improvement, but further than that, I'm not qualified for code review.

dzinelabs’s picture

Is the #4 the only that needs to be applied? And what file needs to be patched?

Dom.’s picture

Patch #4 is the only one that needs appliance, on 8.x-1.x-dev branch.
You should let GIT handle this. See "Apply patch" at https://www.drupal.org/project/back_to_top/git-instructions

dzinelabs’s picture

@Dom, I use netbeans for patching and unfamiliar with Git.

Reuben Unruh’s picture

Status: Needs review » Reviewed & tested by the community

#4 is working. Thanks!

  • sumitmadan authored ecc88ed on 8.x-1.x
    Issue #2638332 by sumitmadan, Dom.: Update 8.x-1.x with Drupal 8 Latest...
sumitmadan’s picture

Status: Reviewed & tested by the community » Fixed

Thanks to all for their contribution. :)

Status: Fixed » Closed (fixed)

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