This theme has a granular set of break-points for responsive design. I have already used for one of my customers. I thought that it should be shared with the community.

Theme page:
https://www.drupal.org/sandbox/drupalfever/2829859

Git Clone Command:
git clone --branch 8.x-1.x https://git.drupal.org/sandbox/drupalfever/2829859.git dfrspnsv
cd dfrspnsv

CommentFileSizeAuthor
#20 dfrspnsv-pareview.jpg663.93 KBe2tha-e
#13 dfrspnsv-drupalcs-result.txt34.1 KBtrangunghoa
#8 Screenshot 2017-02-27 13.34.59.png468.88 KBAndre-B

Comments

drupalfever created an issue. See original summary.

PA robot’s picture

PA robot commented
Issue summary: View changes

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

sandip27’s picture

sandip27 commented
Status: Needs review » Needs work

Hello @drupalfever

I did the automated test for the module and found bunch of issues. Those can be seen at pareview Review.

EDIT: removed long pareview.sh dump.

Please take care of those issues.

Thanks

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

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.

drupalfever’s picture

drupalfever commented
Status: Closed (won't fix) » Needs review

I fixed all the errors that I could. I am talking about the errors described by sandip27.

There is only one error that I didn't understand. I would appreciate any help.

The error that I didn't understand is as follows:


FILE: /root/repos/pareviewsh/pareview_temp/dfrspnsv.theme
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
116 | ERROR | [x] Namespaced classes/interfaces/traits should be
| | referenced with use statements
drupalfever’s picture

drupalfever commented

I just fixed all the errors found at PAReview.
https://pareview.sh/node/404

There are no errors. There is only one warning that I simply don't understand:


FILE: /root/repos/pareviewsh/pareview_temp/dfrspnsv.theme
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
115 | WARNING | Format should be "* Implements hook_foo().", "*
| | Implements hook_foo_BAR_ID_bar() for xyz_bar().",, "*
| | Implements hook_foo_BAR_ID_bar() for
| | xyz-bar.html.twig.", "* Implements hook_foo_BAR_ID_bar()
| | for xyz-bar.tpl.php.", or "* Implements
| | hook_foo_BAR_ID_bar() for block templates."
--------------------------------------------------------------------------

Since this is not an error but just a warning, I think that my theme should be approved.

In any case, if anyone can give me a clue as to what the error message is talking about, I will gladly fix the warning as well.

Following is the code that the warning is complaining about. The following code starts at line 114 of my "dfrspnsv.theme" file:

/**
 * Implements hook_form_alter() to add classes to the search form.
 */
function dfrspnsv_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  if (in_array($form_id, ['search_block_form', 'search_form'])) {
    $key = ($form_id == 'search_block_form') ? 'actions' : 'basic';
    if (!isset($form[$key]['submit']['#attributes'])) {
      $form[$key]['submit']['#attributes'] = new Attribute();
    }
    $form[$key]['submit']['#attributes']->addClass('search-form__submit');
  }
}
Andre-B’s picture

Andre-B
Primary language German
Location Offenburg, Germany
commented

Hello,

/**
 * Implements hook_form_alter() to add classes to the search form.
 */
function dfrspnsv_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  if (in_array($form_id, ['search_block_form', 'search_form'])) {
    $key = ($form_id == 'search_block_form') ? 'actions' : 'basic';
    if (!isset($form[$key]['submit']['#attributes'])) {
      $form[$key]['submit']['#attributes'] = new Attribute();
    }
    $form[$key]['submit']['#attributes']->addClass('search-form__submit');
  }
}

The comment is the issue here, the form for hook implementations must be Implements HOOK. (nothing following on that line)
So fix for that notice is as easy as:

/**
 * Implements hook_form_alter().
 * 
 * Adding classes to the search form.
 */

This is an issue for the following examples:

$ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md .

FILE: /dfrspnsv/dfrspnsv.theme
----------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
  12 | WARNING | Format should be "* Implements hook_foo().", "*
     |         | Implements hook_foo_BAR_ID_bar() for xyz_bar().",,
     |         | "* Implements hook_foo_BAR_ID_bar() for
     |         | xyz-bar.html.twig.", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
  40 | WARNING | Format should be "* Implements hook_foo().", "*
     |         | Implements hook_foo_BAR_ID_bar() for xyz_bar().",,
     |         | "* Implements hook_foo_BAR_ID_bar() for
     |         | xyz-bar.html.twig.", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
 106 | WARNING | Format should be "* Implements hook_foo().", "*
     |         | Implements hook_foo_BAR_ID_bar() for xyz_bar().",,
     |         | "* Implements hook_foo_BAR_ID_bar() for
     |         | xyz-bar.html.twig.", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
 115 | WARNING | Format should be "* Implements hook_foo().", "*
     |         | Implements hook_foo_BAR_ID_bar() for xyz_bar().",,
     |         | "* Implements hook_foo_BAR_ID_bar() for
     |         | xyz-bar.html.twig.", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
----------------------------------------------------------------------

Continued manual review:

1) Good balance of inline comments and code in .theme file.
2) Detailed comments in twig files about available variables etc. - nice.
3) Some more inline comments in css files would be nice.
4) referencing of core assets needs to be done differently - not all themes need to be installed in themes/ directly same goes for modules. Any subfolder logic can be applied - if that's the case referencing like background: url(../../../../misc/icons/000000/chevron-left.svg) will cause 404s. See more examples in the screenshot attached.

Andre-B’s picture

Andre-B
Primary language German
Location Offenburg, Germany
commented
StatusFileSize
new Screenshot 2017-02-27 13.34.59.png468.88 KB
Andre-B’s picture

Andre-B
Primary language German
Location Offenburg, Germany
commented
Status: Needs review » Reviewed & tested by the community

I don't see blockers here.

drupalfever’s picture

drupalfever commented

Hello, Andre-B! Thanks for the great amount of time spent on evaluating my theme.

Great stuff there. I will take my time going over all your suggestions.

Once again, I appreciate all your help!

drupalfever’s picture

drupalfever commented
Status: Reviewed & tested by the community » Needs review

Hello, Andre-B! Thanks to you, my code is now passing the PAReview.sh test with flying colors!

https://pareview.sh/node/1203

By the way, just altering the comment to what you suggested did not work:

/**
 * Implements hook_form_alter().
 * 
 * Adding classes to the search form.
 */

I had to remove any other comment. I could only leave the "Implements hook_" line:

/**
 * Implements hook_form_alter().
 */

With regards to your CSS suggestion, I do not know how I could address the problem of referencing to images in any other way.

I would be forever thankful :) if you could give me any suggestion.

trangunghoa’s picture

trangunghoa commented

Hey @drupalfever,
I used command: git clone --branch 8.x-1.x https://git.drupal.org/sandbox/drupalfever/2829859.git dfrspnsv
and there are my results:

Automated Review
https://pareview.sh/node/404
I just saw one error for your files.
"End of line character is invalid; expected "\n" but
| | found "\r\n""
Because you used the style default by the window.

Manual Review
Individual user accounts
No
No duplication
No, Not sure, but I think it's duplication by the Bartik themes, please explain to me.
Master Branch
Yes.
Licensing
Yes.
3rd party assets/code
Yes.
README.txt/README.md
Yes.
Code long/complex enough for review
Yes.
Secure code
Yes

trangunghoa’s picture

trangunghoa commented
StatusFileSize
new dfrspnsv-drupalcs-result.txt34.1 KB
drupalfever’s picture

drupalfever commented

Hi, trangunghoa! Thanks for reviewing my theme.

As I said before, the PAReview.sh site does not show any error. I just followed the link that you shared with me in your comment and it is no complaining about anything.
https://pareview.sh/node/404

If there was a problem with line-breaks with \r\n instead of \n, I think that PAReview.sh would have complained about it, right?

I have noticed that you are using a different code reviewing software. I opened the text file that you shared with your report and will see what I can do about these warnings.
dfrspnsv-drupalcs-result.txt

No duplication
No, Not sure, but I think it's duplication by the Bartik themes, please explain to me.

I have noticed that you are having second thoughts about the originality of my theme. You are absolutely right! In fact, I based my theme on the Bartik theme. I had to start somewhere and I didn't want to start a new theme from scratch.

I like the Bartik theme! However, I was quite disappointed when I decided to use this theme for one of my clients. I didn't like how the website behaved when viewed by mobile devices. I thought that the breakpoints were set in a weird place. In my point of view, the mobile friendliness of the theme was beyond practical use.

As a result, I spent an inordinate amount of time tweaking the CSS and template files. I finally got the theme to a point where I felt happy with the final result. I have been using this customized theme ever since.

That's when I decided that it was time to share the fruit of my labor with the rest of the community. So, here we are! :)

Once again, I would like to thank you for taking the time to review my theme. I appreciate it. It means a lot to me!

drupalfever’s picture

drupalfever commented

Hi, trangunghoa! I managed to edit the following file:

        modified:   color/color.inc
        modified:   color/preview.js
        modified:   css/colors.css
        modified:   css/components/block.css
        modified:   css/components/book.css
        modified:   css/components/breadcrumb.css
        modified:   css/components/buttons.css
        modified:   css/components/contextual.css
        modified:   css/components/demo-block.css
        modified:   css/components/dropbutton.component.css
        modified:   css/components/featured-bottom.css
        modified:   css/components/featured-top.css
        modified:   css/components/feed-icon.css
        modified:   css/components/field.css
        modified:   css/components/form.css
        modified:   css/components/forum.css
        modified:   css/components/help.css
        modified:   css/components/highlighted.css
        modified:   css/components/image-button.css
        modified:   css/components/item-list.css
        modified:   css/components/list-group.css
        modified:   css/components/list.css
        modified:   css/components/main-content.css
        modified:   css/components/menu.css
        modified:   css/components/messages.css
        modified:   css/components/node-preview.css
        modified:   css/components/node.css
        modified:   css/components/page-title.css
        modified:   css/components/pager.css
        modified:   css/components/panel.css
        modified:   css/components/password-suggestions.css
        modified:   css/components/search-form.css
        modified:   css/components/search-results.css
        modified:   css/components/shortcut.css
        modified:   css/components/sidebar.css
        modified:   css/components/site-branding.css
        modified:   css/components/site-footer.css
        modified:   css/components/skip-link.css
        modified:   css/components/table.css
        modified:   css/components/tablesort-indicator.css
        modified:   css/components/tabs.css
        modified:   css/components/toolbar.css
        modified:   css/components/ui-dialog.css
        modified:   css/components/ui.widget.css
        modified:   css/components/vertical-tabs.component.css
        modified:   css/maintenance-page.css
        modified:   css/print.css
        modified:   dfrspnsv.theme

I have replaced the "\r\n" with "\n".

Would you mind checking my files once again?

Andre-B’s picture

Andre-B
Primary language German
Location Offenburg, Germany
commented

Hello,
On pareview is a retry button: https://pareview.sh/node/404

Its down to two notices, regarding the core assets- you should be able to reference them starting from the root directory like '/misc...' (I am on my mobile right now so cant check the correct path)

trangunghoa’s picture

trangunghoa commented

Hi drupalfever,
I didn't see anything change, maybe you didn't submit the change.
For your module I think you should get another UI instead of Bartik because it does not make much sense.
I know you have spent much time to change the CSS and layout, but users will check the UI first.

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxdrupalfever2829859git

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

drupalfever’s picture

drupalfever commented
Status: Needs work » Needs review

Hi, trangunghoa!

I didn't see anything change, maybe you didn't submit the change.

I made changes on my GIT configuration and on my Eclipse PDT configuration so they don't add \r\n to my files anymore. I also forced GIT to update the aforementioned files. I believe that you will be able to see the changes now.

For your module I think you should get another UI instead of Bartik because it does not make much sense.

I apologist but I did not understand your statement. What does not make sense? Well, I think that the Bartik UI is really good. I don't want to make changes to a part of the theme that was well designed. My goal was to build upon what was already a good theme.

I know you have spent much time to change the CSS and layout, but users will check the UI first.

I am not trying to hide the fact that my theme was based on Bartik. In fact, I think that this is an asset. Users will feel right at home using my modified theme since the Bartik theme is so popular.

I am not sure if you understand the spirit of my intentions when I decided to submit this theme. As the description of this theme states, this theme has an improved and more granular set of break-points for responsive design websites.

I don't think that I should spend time changing the rest of my theme just to make it different enough from the original code. I would be spending energy on an unworthy cause. I don't think that there is anything wrong with having a theme that is similar to another one. That's the point of open source. We are supposed to stand on the shoulders of giants.

I believe that the changes that I made on the Bartik theme are worthy of sharing with the rest of the community. My only hope is that the community will allow me to prove that this theme has the potential to be useful to others.

There is only one way to know for sure whether my theme will be well received or not. You can only answer this question by allowing me to make this theme into a full project.

I am hoping that you will.
:)

Once again, I thank you for the time spent on reviewing my project.

e2tha-e’s picture

e2tha-e commented
StatusFileSize
new dfrspnsv-pareview.jpg663.93 KB

drupalfever ,
This theme still gets a bunch of errors when run through pareview.sh.

pareview screenshot

They are all this error:

Short array syntax must be used to define arrays

It's just a coding standard which the Drupal community deems significant enough to call an error when not adhered to. But in any case, your team looks comfortable with the short array syntax, even within the same file using the long syntax.

color/color.inc lines 122-129:

  '#attached' => [
    'drupalSettings' => [
      'color' => [
        // Put the logo path into JavaScript for the live preview.
        'logo' => theme_get_setting('logo.url', 'dfrspnsv'),
      ],
    ],
  ],

Might as well just use the short syntax consistently.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work

I take the last comment forget to change status, since it is reporting code to change.

e2tha-e’s picture

e2tha-e commented

@kiamlaluno You are correct. Thanks!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Closed (won't fix)

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.

I am closing this application due to lack of activity.