Port request: views_timelinejs

[tiagoassis]

Name of the module, theme, or layout views_timelinejs

Link to the drupal.org module, theme, or layout https://www.drupal.org/project/views_timelinejs

Link to D7 source code https://git.drupalcode.org/project/views_timelinejs/tree/7.x-3.1

[rudy880719]

Hello, I have successfully ported the Views_timelinejs module from Drupal to Backdrop CMS. You can find my contribution to the Backdrop CMS community on GitHub at the following link: Views-TimelineJS-Backdrop-Port. I would appreciate any feedback on this module.

[laryn]

@rudy880719 That's great, I think this module could be helpful to people. Thank you for working on it!

I haven't looked in depth or done any review/testing of the code, but I can see that there are a few issues before it can be considered for inclusion in the Backdrop contrib space. Here are a few I see at a glance:

• the commit history from the Drupal 7 project needs to be included
• the README should be formatted as in the example provided (see link below)

• Maintain the Git history from Drupal 7. See this article.
• Include a README.md file that includes license and maintainer information. You can use this example.

[rudy880719]

@laryn Thank you for your instructions, the problems were solved, please check the url again Views-TimelineJS-Backdrop-Port.

[laryn]

@rudy880719 I have a few suggestions for your consideration (these are not requirements, just my review/opinion):

• Since you're working from 7.x-3.x in Drupal, I'd recommend using 1.x-3.x as the branch in Backdrop, and starting out with a matching release number to make it easy to tell where it fits in terms of the Drupal version. (e.g. a straight port from 7.x-3.1 would make sense as 1.x-3.1.0 in Backdrop's numbering system).
• Optionally, since you have a namespaced configuration file, you don't need to namespace every setting as well. (e.g. views_timelinejs_css_library_group could be tightened up to css_library_group.
• Functions like this should be removed: views_timelinejs_update_7300
• You can get rid of views_timelinejs_uninstall completely as config owned by a module is automatically cleared when the module is uninstalled.
• Likewise I think views_timelinejs_install can be removed unless there's something that needs to happen there.
• The css_library_group value should be set directly rather than using the CSS_DEFAULT constant, since that may not be available in the install file or default config. It looks like that's just a 0.
• Rather than remove the local options completely, I wonder if you could bundle the library locally as an option that wouldn't require an internet connection. From what I can tell, the license would allow this -- do you read it the same?
• You may be able to just use debug instead of all the views_timeline_debug code.

[rudy880719]

@laryn Thanks for the suggestions. I have solved all of them, I appreciate your collaboration. If everything is fine, please let me know what is the next step.

[rudy880719]

@laryn Hello, please, let me know if the code is fine to continue with others ports

[argiepiano]

Hi @rudy880719.

• Get rid of Drupal mentions in the README.md file
• I noticed the module looks for the local library inside $base_url . '/sites/all/libraries/' (this is also mentioned in the README). While this may work, please notice that Backdrop typically doesn't use the same installation paths as Drupal, when you have a single-site installation. Single installations usually have the settings.php file in the root of the installation, and all site-related modules such as modules, themes, layouts, etc in the root as well. It would be weird to force the site developer to create the sites/all/libraries/ folder just for this library. Instead, I'd suggest you direct the user to create a libraries folder on the root of the installation, and place the files there (if they want to have a local version). Alternatively, you could provide the library yourself within the module's folder (views_timelinejs/libraries) - this is pretty standard in Backdrop.

I also see a problem with views_timelinejs_update_1000(). @laryn suggested that you don't use namespacing such as views_timelinejs_css_library_group in the config. HOWEVER, the Drupal variables ARE namespaced, so these lines need to be changed to the following:

/**
 * Migrate views_timelinejs variables to config.
 */
function views_timelinejs_update_1000() {
  $config = config('views_timelinejs.settings');
  $config->set('library', update_variable_get('views_timelinejs_library', 'cdn'));
  $config->set('css_library_group', update_variable_get('views_timelinejs_css_library_group', 0));
  $config->save();

  update_variable_del('views_timelinejs_library');
  update_variable_del('views_timelinejs_css_library_group');
}

[argiepiano]

Edited my comment above for accuracy - sorry for the typos

[rudy880719]

@argiepiano Thanks for the suggestions, could you review de code again, I fixed al the issues.

[argiepiano]

I think you misunderstood my comment about namespacing. You MUST namespace the Drupal 7 variable, but the Backdrop config setting does not need the namespacing. Please do take a look at the code I posted above. Notice that, for example:

$config->set('library', update_variable_get('views_timelinejs_library', 'cdn'));

the library setting is not namepaced, only the variable you are getting from the D7 installation during upgrade: update_variable_get('views_timelinejs_library', 'cdn')).

One more thing. Your default repo branch should be 1.x-3.x, not 1.x-3.1.0. Your releases will be fully named like that, but the default branch (i.e. the development branch) should not be named with the actual minor.patch numbers.

[rudy880719]

Thanks again @argiepiano, changes are already in current branch.

[argiepiano]

Thanks for the quick reply! Things are looking good. This is a suggestion - not required. I see you are using the #config FAPI property in views_timelinejs_settings_form(). This, however, has no effect, unless you run the $form array through system_settings_form(). See the examples in the FAPI tutorial page.

So, if you wanted to modify this way, then you would not need the submit handler. Backdrop would take care of updating the config file. BUT just be sure to NOT namespace the key of the $form array, as in:

$form['library'] = array( so that Backdrop saves the value to the library config setting inside views_timelinejs.settings.json

[djzwerg]

Because you are working in this @rudy880719 I think my PR could be closed: https://github.com/backdrop-ops/contrib/issues/681

[rudy880719]

project transfered to backdrop-contrib and created new release https://backdropcms.org/project/views_timelinejs