[D8] BonVoyage theme

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2662738

Project 2: https://www.drupal.org/node/2369009

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

Git clone failed for http://git.drupal.org/sandbox/webtitan/2662716.git while invoking http://pareview.sh/pareview/httpgitdrupalorgsandboxwebtitan2662716git

Git repository is empty. Aborting.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

I can't download given clone url git clone --branch 8.x-1.x http://git.drupal.org/sandbox/webtitan/2662716.git
bon_voyage, here i reviewed with git clone --branch master https://git.drupal.org/sandbox/webtitan/2662716.git bonvoyage

Automated Review

Please fix the pareview issues http://pareview.sh/pareview/httpgitdrupalorgsandboxwebtitan2662716git

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[ No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Major finding, needs work

page.html.twig file contains the following code,

<li id="header_link_signin">
						<strong><a href="/user" title="signin">signin</a></strong>
					</li>
					<li id="header_link_contact">
						<strong><a href="/contact" title="contact">contact</a></strong>
					</li>

which contains static link, Please fix this.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Hi,

I've made some manual testing and review of bonvoyage theme

Testing your theme

1- The scroll of the front page of your demo site is really very very fast. It’s really difficult to scroll to the middle of the page.
2- Image are not limited to their container and so may overlap the design. because there is no css rule on img with max-width: 100%
3- Your theme do not purpose to add the page title to the breadcrumb ? Adding page title or not to breadcrumb is now the responsibility of theme.
4- I can’t change the site’s logo in the settings theme. I have to do it in the global settings.
5- The branding block is not anymore functional. Site name and site slogan are not displayed if these options are checked.

Manual review

stick-up.js
6- Don’t use jQuery(document).ready(function () but use Drupal.behaviors instead

menu—main.html.twig
7- You can remove unsed lines in your template

Header menu
8- The header menu is hardcoded in the page.html.twig. It’s not really a best practice to harcode menu in a template. Why not use the user menu ? Further more, even logged users see the signin menu. Why do not you use the account menu provided by core which handle nicely login/logout behavior.

FontAwesome
9- Your theme include the fontAwesome Font Icon. FA is under the SIL OFL 1.1 licence, which is not GPL compatible. See https://www.drupal.org/node/2139273

README
10- The bootstrap version used is 3.3.6. Not the 3.2 mentioned in the README.

forgot to change status

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

hello!
as a general code review comment. your JS behaviors should state 'use strict' as in include the other js object dependencies that your using.

IE: cores dialog.js file

as well, you shouldn't need to state $(document).ready() within as the behavior system basically already does this.

a theme is a big task to take on as a contrib project. Everyone will point out every browser issue soon enough.

remember to keep it simple at first... build up ability over minor releases.
best luck.

I've installed bon_voyage theme. If I go to /admin/appearance/settings/bon_voyage, I've got a fatal error

Recoverable fatal error : Argument 2 passed to bon_voyage_form_system_theme_settings_alter() must be an instance of FormStateInterface, instance of Drupal\Core\Form\FormState given, called in /drupal8.local/core/modules/system/src/Form/ThemeSettingsForm.php on line 326 and defined dans bon_voyage_form_system_theme_settings_alter() (ligne 25 de /drupal8.local/themes/bon_voyage/bon_voyage.theme).

re: #35
the "must be an instance of formStateInface" -- maybe because you are missing a 'use' statement .
PS: your 'use' statement on line 88 should be at the top of the file!!!

i'm not sure why pareview isn't picking that up... but it is a bot--they aren't perfect (:

yes. you have not declared the class FormStateInterface in your file .theme. You shoud put at the top of the file
use Drupal\Core\Form\FormStateInterface;

Automated Review

Re-ran the test. Looks good: http://pareview.sh/pareview/httpgitdrupalorgsandboxwebtitan2662716git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code.

I'm not an expert in licencing, but I see that FontAwesome is included. Looking at http://fontawesome.io/license/ it states that the fonts of FontAwesome are licenced as SIL OFL 1.1. This license is listed here as not compatible with GPLv2: https://www.drupal.org/node/1475972#whitelist-licenses . Maybe you can include the option to import it via Bower? http://bower.io/

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.

1. README.txt provides enough information, though it doesn't strictly follow the template (I don't know how strict this requirement is).
2. Demo site in readme file doesn't work: http://africannorthwest.com . Fix either demo site or remove URL.
3. Under "Drupal compatibility" you state compatibility with "Drupal Drupal 8.x-1.x". I assume it's compatible with any version of Drupal 8, so just stating compatibility with Drupal 8 is fine.
4. Under "Support & Customization" an email address is listed. Preferably all support happens in the Drupal issue queue, unless it's paid customizations. That's probably what you meant - maybe that should be made clearer.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. bon_voyage.info.yml: Use a human readable name under "name". For example "Bon Voyage", or I've seen it referred to as "BonVoyage" elsewhere in the code.
2. bon_voyage.info.yml: Add a little longer description, and use a human readable name.
3. bon_voyage.info.yml: Inconsistent use of single quotes around region labels.
4. bon_voyage.theme: Function comment for 'bon_voyage_preprocess_page', should be 'Implements hook_preprocess_page().'
5. bon_voyage.theme: There is a few title or label strings that have every word capitalized. Just use capitalization on the first word.
6. bon_voyage.theme: L90: only leave one line between functions
7. For class names, always use dashes between words instead of CamelCase: https://www.drupal.org/coding-standards/css/architecture#formatting
8. to-top.css: #toTop:before {} text-align property used twice.
9. to-top.css: #toTop:after {} Faulty color value on border property
10. to-top.css: #toTop:hover {} use 6 character hex value (check this on other css files too)
11. style.css: @charset should be on the first line
12. style.css: #main-navbar ul.navbar-nav > li > a {} duplicate transition property
13. style.css: #main-navbar ul.navbar-nav > li > a:before {} if a value is 0, no need to add value type like 0%
14. style.css: #block-searchform #search-block-form .btn.button-search:after {} Duplicate use of -webkit-transition property.
15. check in all template files: use double space instead of tabs.
16. page.html.twig: L74 Pass "Categories" string through Twig translate function
17. The demo at http://sfast-fwd.net/ has really bad scroll hijacking, which makes the site unusable for me, though I didn't see it on a local install, so I assume this is fixed and the demo is out of date.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Overall everything I noted here is very minor, except for the FontAwesome licencing.

This review uses the Project Application Review Template.

PAReview: 3rd party code

Font Awesome appears to be 3rd party code/content. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

@webtitan

- Glyphicons aren't allowed either. It even says so on their license page: http://glyphicons.com/license/ "You must not: include icons in open-source projects, since this license is not compatible with most of the open-source licenses"

- As for the scrolling script: I see this behaviour in both Firefox and Chrome on a OSX 10.11.4 Mac. On IOS Safari it works fine though. What happens is that scrolling using the trackpad is extremely sensitive. Where a light scroll normally moves the page about 20px down, on your demo site it scrolls down half the page, making the site unusable. As mentioned in #42, that's only on the demo site.

Yes, please remove all third party content from the git repository and tell users where to download it instead.

found some issues with z-index property on main navigation and admin toolbar module. See picture attached.

"all third party content" also includes font-awesome.css found in the /css directory. Please call it via CDN as you have done for bootstrap.

Besides, the google font is SIL OFL license. It can be called via CDN as well in the libraries.yml file.

Also, I would recommend that you add a simple composer.json file to define your project as a "theme", which will be helpful for those who may use drupal-composer/drupal-project.

In the .libraries.yml file, both global-styling and bootstrap has dependencies set. Do we really require these?
a) Google font irrespective of core jquery should work.
b) Bootstrap is being called from CDN. So no question of any core jquery dependency.

fix these and we go RTBC :)

thanks for incorporating all the requested changes till here with patience.. :)

Thanks for your contribution, webtitan! You are now a vetted Git user. You can promote this to a full project. Always exciting to get contributed themes!

When you create new projects (typically as a sandbox to start) you can then promote them to a full project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues (here you get to ask fans of your project to help you!)

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process, especially my slowness in approving following review. We know it's broken and are trying to fix it.