[D8] Theme Breeze

Hi ikit-claw,

Pareview.sh is giving following errors.

http://pareview.sh/pareview/httpgitdrupalorgsandboxikit-claw2710867git

Will do a manual review in some time.

Also please add module name after the git clone url so that it will get cloned easily to module name folder.

Hi ikit-claw,

Thanks a lot for submitting this theme. Here's a few comments on having a look at your theme.

1) Change your git clone command to
git clone http://git.drupal.org/sandbox/ikit-claw/2710867.git breeze
This will mean that the folder will be renamed from 2710867 to breeze when downloaded.
2) in your breeze.theme file you have 4 spaces for indenting code. Drupal coding standards need two. I'd suspect this might also be the case in other files - I'll let codesniffer/pareview check that for you.
3) In breeze.libraries.yml file you have 5 spaces where you need 6 and 3 spaces where you need 4. See for example lines 4 and 12 (YAML is very picky about spaces).
4) In breeze.libraries.yml you have spaces at the end of lines - remove these.
5) In breeze.info.yml you have space before the comment on line 9
6) I think you should delete the breeze.info file - it is superceded by the breeze.info.yml file
7) In html.html.twig you are calling google fonts explicitly. This info should be called via a library in Drupal 8 - see the "CDN/External Libraries" section here: https://www.drupal.org/theme-guide/8/assets
8) In page.html.twig you have spaces after you open a class and before you close it, for example on lines 78 - 87
9) In your JS folder you have bootstrap.js and bootstrap.min.js - are both needed? If you remove one, make sure to update your breeze.libraries.yml file
10) script.js should be closed properly and inside a strict mode - see here: https://www.drupal.org/node/172169#strict
11) Lots of indentation errors in your style.css file - 4 spaces used where there should be two. Same seems true for your other CSS files. (Was this theme created with SASS or another preprocessor? - would be nice to have those files).

Most of the errors you got from pareview are correct, as most of them are indentation errors. Please read up on Drupal coding standards for indenting and whitespace here: https://www.drupal.org/coding-standards#indenting

I haven't installed the theme, so these comments are all based on reading the code. Others may file some more issues for you after they install the theme.

Thanks again for submitting this to drupal.org.

Nice theme. Just noticed there is no README.txt, as required in the manual review guide.

Well done to you. It takes a bit of time to go through this process, but fair play to you for persevering.

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.

Tested. It installs and works fine

This theme has not been "Reviewed and tested by the community". I've left a review comment above (based only on reading the code, without having installed it - as mentioned in my comment). I've also been giving ikit-claw a lot of help on Slack to get it ready to be reviewed properly and tested by the community.

I have the theme installed now and am going to do some review work on it. Please do not mark this as 'Reviewed and tested by the community' unless you have reviewed it (as opposed to just installed it) - this lessens the quality of code on Drupal.org for others.

As a first item that should be fixed - the footer message is hardcoded. It would be nice to have this as a variable on the theme settings page (presuming most people who install this theme will not want a message saying it is a 'demo' site on their live site.

Setting back to "Needs work".

Some comments:

- TinyNav.js only works when the menu is placed in the 'navigation' region. This is not a show-stopper. However, dropdown menus being select lists is not best practice.
- The draggable handles on the block placement page seem obscured
- Buttons don't seem to have any hover/focus states - this is an accessibility issue
- Breadcrumbs are set in an <ol> - this should be a <ul> and display of each <li> in this probably should be inline.
- A small bit of space between the vertical tabs and the vertical tabs content would be appreciated. At the moment the content pushes flush against the tabs
- You have have a more filled out description for your theme than "A Drupal 8 Theme".
- If you are not using some of the bootstrap css, please delete the commented-out lines in breeze.libraries.yml
- Spacing errors in breeze.theme - use 2 spaces for indenting, not 4
- Same for script.js
- Check indentation on in html.html.twig
- In same file, you have user-scalable set to "no". This is an accessibility concern. Users should be allowed to zoom in to read the content if they need to
- Do not hard code footer messages - or at least not ones saying every site is a demo site - a link back to OS Training instead would be fine.

@markconroy: <ol> is fine for breadcrumbs. I'm not saying it's better than <ul>, just that there's nothing wrong with it as a choice. The breadcrumb trail represents a directional sequence of steps, so some folk prefer <ol>, and it's not a show-stopper here.

Some articles about using <ol> for breadcrumbs. It has a posse :-)

• CSS-Tricks: Exploring Markup for Breadcrumbs
• Creating a semantic breadcrumb using HTML5 microdata
• Breadcrumbs are in a specific order so using an "ordered" list seems appropriate
• Notably in this case, the Bootstrap components documentation uses <ol> for the breadcrumb.

Re: #22

In same file, you have user-scalable set to "no". This is an accessibility concern. Users should be allowed to zoom in to read the content if they need to.

Agreed. However, the viewport meta tag also includes a maximum-scale=1.0 which needs to be removed as well.

I recommend:

<meta name="viewport" content="width=device-width, initial-scale=1">

These also happens to be the viewport options used by HTML5 boilerplate and Bootstrap's getting started tutorial.

The branch names in the repo are confusing.

Most work has happened in the 8.1.x-1.x, but this is not the correct format. See the Release naming conventions.

8.x-1.x is the correct version numbering format, but it only has 2 commits from April.

I recommend merging the 8.1.x-1.x into 8.x-1.x. This will be important later on when the time comes to make packaged releases, as only correctly named branches/tags will be recognized by the packaging system.

See:

• Creating a branch or tag in Git
• Creating a project release

(If you meant to imply that this theme is only compatible with Drupal Core v8.1, this isn't something you can indicate with the version number. You should state that on the project page instead.)

re: #22

Buttons don't seem to have any hover/focus states - this is an accessibility issue

Missing focus styles are indeed an accessibility issue, specifically for WCAG success criterion 2.4.7 Focus Visible. You'll be helping lots of people by including them.

Missing hover styles are not an accessibility issue though; users follow the mouse pointer icon, and hover styles are a nice supplement to this.

The Bootstrap library and TinyNav script are third party components licensed under an MIT license variant. Sadly, these are going to have to be removed :-(

The Drupal Git Repository Usage policy says:

All files checked into the repository (code and assets) must be licensed under GNU/GPL version 2 and later.

[...]

DO NOT include code from a non-Drupal project in the repository. If your module requires non-Drupal code, such as a third-party JavaScript/PHP library, provide a link to where the other code can be downloaded and instructions on how to install it.

The existing Bootstrap base theme does not package the Bootstrap library with the theme. Instead it has instructions in a README file (for the starterkit) advising users to download it, and where it should be placed.

For a D8 theme, you CAN declare these in your *.libraries.yml file. See the D8 Bootstrap base theme for an example of how to do this. Basically your library declarations will expect the third party files to be in the right place. The starterkit README from the Bootstrap base theme instructs users where to place them after download.

This aspect of third-party libraries is all a bit of a pain, I'm afraid. Please take this seriously though, as it's a blocker to promoting this project to full status.

There's some useful further reading at:

• The Drupal Licensing FAQ
• 3rd party libraries and content on Drupal.org
• Why drupal.org doesn't host GPL-"compatible" code

Re: #31

it says to use 8.1.x-1.x I guess that page is outdated and needs updating ?

The content on the git instructions is dynamic, it says 8.1.x-1.x when that's the branch selected at the start of the page ("version to work from").

Re: #31

I've merged I think... Haven't deleted because I am not sure I did it right.

You did it right. You can check this yourself using git log branch-A..branch-B, which will show all commits that are present in branch-B but not in branch-A.

Like so...

• git log 8.x-1.x..8.1.x-1.x says there are NO commits in 8.1.x-1.x that are not also present in 8.x-1.x, and vice-versa...
• git log 8.1.x-1.x..8.x-1.x shows there are currently 9 commits in 8.x-1.x that are not in 8.1.x-1.x,

Which means the merge went right. git log --decorate 8.x-1.x is also informative.

The 8.1.x-1.x branch can now be deleted:

1. git branch -d 8.1.x-1.x will delete your local copy, and
2. git push origin :8.1.x-1.x will delete the branch from the drupal.org repo (literally "push NOTHING to the upstream branch, effectively deleting it). Note: you might get an error if 8.1.x-1.x is set as the default branch on drupal.org, so set 8.x-1.x as the default branch on the project node (under edit > default branch).

Hi ikit-claw,

I was going to ask Andrew if he felt like doing some mentoring on this theme this evening. Hadn't realised he had the same plan. Here's my thoughts.

I don't like the table layout on admin pages, for example on the 'admin/structure/blocks' page. This table should be 100% width. It's not a blocker, but I'd like to see it more usable in a future release of the theme.

The draggable handles issue is there still. I'm not sure if that's a blocker - @andrewmacpherson will confirm if it's an accessibility blocker or not.

Some of my other issues mentioned above seem to not have been addressed. The accessibility issues in this list must be addressed.

- Buttons don't seem to have any hover/focus states - this is an accessibility issue
- Breadcrumbs are set in an <ol> - this should be a <ul> and display of each <li> in this probably should be inline. (though this is a preference, not a requirement)
- A small bit of space between the vertical tabs and the vertical tabs content would be appreciated. At the moment the content pushes flush against the tabs (again, this will help with readability)
- You should have a more filled out description for your theme than "A Drupal 8 Theme". (not necessary, but would be nice)
- If you are not using some of the bootstrap css, please delete the commented-out lines in breeze.libraries.yml
- Spacing errors in breeze.theme - use 2 spaces for indenting, not 4
- Same for script.js (some spaces here use 3 spaces)

The draggable table handles look like they might be obscured (or clipped?) in that screenshot, which I expect will make them a tricky small target for mouse users, as well as not being recognizable as a 4-arrows icon. A general usability issue, not accessibility specifically.

The lack of focus styles is certainly an accessibility issue.

I'm agnostic about OL vs UL for breadcrumbs - it's the maintainer's call.

Andrew - does this mean that the only issue we should require must be fixed before the theme can be set as RTBC is the focus styles for buttons and the indentations in (for example) script.js?

If so, that's great. I think this theme is almost ready for promotion and the other issues can be fixed in later releases at ikit-claw's convenience.

The draggable handles are now working. It seems to me that the only issue that must be attended to now is the :focus styles on buttons.

After that I am happy to move to RTBC (as long as Andrew is as well - presuming no other accessibility blockers).

Thanks for your effort and time and patience ikit-claw.

There's an a11y issue with the user-login block, where the label elements have blank text. The #title text is being removed in breeze_form_alter(). Form inputs need programatically associated labels, and the placeholder attribute should not be used as a substitute.

See Placeholder Attribute Is Not A Label! and HTML5 Accessibility Chops: the placeholder attribute for guidance.

To fix this easily, use the Form API #title_display property instead:

      $form['name']['#attributes']['placeholder'] = $form['name']['#title'];
      $form['name']['#title_display'] = 'invisible';
      // ... likewise for password field.

This puts a visually-hidden class on the label element. I tried this, and your visual styling is unaffected, as your icons are being added to the wrapper div using the ::before pseudo-element.

When the search form block is placed in the right header region, the submit button is hidden using CSS display:none. This prevents assistive technology like screen readers from knowing about the button.

A better way of hiding the button visually is to add the visually-hidden class via a form alter.

I notice that this button does not get hidden when the search form block is placed in another region like the sidebar - is this intentional? If so, a form alter would be an awkward way to hide it, as you would have to deduce which region it was in.

An alternative way would be to use the same styling rules as the visually-hidden class, but duplicated in your own selector, like so:

.header-right .block #search-block-form .form-actions {
  clip: rect(1px, 1px, 1px, 1px);
  height: 1px;
  overflow: hidden;
  position: absolute !important;
  width: 1px;
  word-wrap: normal;
}

Required form elements are not being indicated visually - a sighted user has no way to know which fields are mandatory.

In Bartik a red asterisk is used, which comes from Classy's form.css (see .form-required::after{}).

TinyNav is a "jump menu" navigation widget, powered by JS onChange event. You should be aware that these have various accessibility problems. This blog entry by Paul J Adam (well worth following...) gives a fairly complete run-down of the issue - OnChange Event on a Select Input/Jump Menu Accessibility Problems.

The upshot is that some keyboard users may face serious (brick-wall) barrier using the jump menu, depending on what browser (and/or screen reader) they use. Also, I have no idea whether users of switch control will be affected.

Unfortunately there's no easy way to fix it in the Drupal theme - it's really an upstream problem for TinyNav. As the linked blog article says, a more accessible (not perfect...) jump menu may be possible, by avoiding the onChange event. However I'm not aware of a suitable JS plugin which would provide an easy replacement for TinyNav.

So, I wouldn't say this is a blocker to RTBC for your breeze theme, but it be worth:

• Documenting the problem on the project page (and README),
• Adding a theme setting to allow site builders to decide whether to use TinyNav,
• Considering alternatives to a jump menu widget.

The CSS library info declares the bootstrap.css.map files - these don't have any use on a production site, so they should probably be removed.

[rant]

Apologies in advance for the rant.

While we appreciate the theme reviewers, this has been a long, exhausting review process that's dragged out from the start of July into October.

Now we see other themes published instantly and they don't even have code:
https://www.drupal.org/project/eleven
https://www.drupal.org/project/sensible

How? Why?

Daniel has had an incredible level of patience, but after seeing those themes published, my level of frustration is up to 11.

This broken process has real consequences: https://www.ostraining.com/blog/drupal/drupal-org-themes/

[/rant]

You're probably tired of hearing it but that menu does seem to be problematic. You mentioned replacing it: have you started working on that yet? While you're at it I'd make some changes to that to make it more readable... which lead me to..
The contrast on a few things seemed difficult for me to read so I did a quick check using this:
https://chrome.google.com/webstore/detail/accessibility-developer-t/fpkk... and it agreed there were a few spots that needed help in that department.

I'll try to give this a more detailed look through later.

@steveburge

Your rant is not helpful and does nothing to speed things up (just like saying in a very early comment that the theme was fine when clearly it wasn't).

We all know the process for getting a project promoted to full status is a long one - though 3 months for a theme for a first time contributor is not a huge amount of time. It took me over a year, submitting a number of projects. I'm glad it did, as I am a better developer because of it and now feel comfortable mentoring people like Daniel to help them get their projects promoted to full status (in my free time, may I add?).

Though I don't think your question deserves a reply given it shows a complete ignorance of how the process works, I'll endeavour to answer anyway - in case anyone comes across this in the future (all part of being a volunteer mentor, I guess). Once a developer is given git access to Drupal.org he/she is then free to create projects on the site and have them at full status instantly. So, once Daniel has this credit, he can create new projects and promote them to full status. The two that you mentioned were created by people who have git access. I presume they created those to hold the namespace for themselves and will develop themes on those namespaces as time goes on.

Here's how the process works, in case you want to know more:
Where Daniel is:
Apply for permission to create full projects
Where Daniel will be soon:
Promoting sandbox projects to full projects
And for extra-bonus points reading, we are trying to fix the application process, but it takes time:
[policy, no patch] Changes to the project application review process

In future, instead of a pre-emptive apology for a rant, how about you don't have the rant in the first place?
Be courteous. Be patient (especially with volunteers).

I've looked through all of the code. This theme is fine. While there are functionality things that ought to be fixed, there is nothing that should block this from being approved.

One minor thing I saw: the custom JS file is formatted incorrectly.

I'm really sorry this took so long. :-(

Apologies, found a few other minor thing making a total of four things that need a quick fix:

• js/script.js needs to be reformatted to match the coding standards, mainly that it should use two spaces to indent.
• templates/comment.html.twig has incorrect indenting for the footer and div sections, i.e. lines 77 through 104 are indented two spaces too many.
• templates/menu-local-tasks.html.twig line 12 needs a *.
• templates/node.html.twig line 7 needs to wrap at 8 characters.

Sorry about the confusion, but if you fix these few things I'll put it back to RTBC.

@damienmcKenna Many thanks. We'll get working on those fixes.

@markconroy My apologies for the rant. We really appreciate the volunteer efforts.

Stepping back and looking at the bigger picture, the approval process seems really broken. Correct me if I'm wrong, but there it seems as it zero themes have been approved in 2016.

The last successful Drupal theme applications seem to have been in 2015 ...
D8: https://www.drupal.org/project/liberty
D7: https://www.drupal.org/project/auro

That seems so odd that I must be missing something?

Hi @steveburges,

I don't think you're missing anything. Not many themes in general get submitted for approval (towards modules). Now that we are on Drupal 8 which was only released recently I guess everyone is trying to learn how it works and how the new theming system works - using Twig.

When that knowledge is gained, more themes may be submitted. I wouldn't hold my breath. It seems to me that most Drupal developers like to work on large projects and most large projects want custom themes. That's what I spend my days working on. WordPress cleans up on the small projects where more people are happy to have similar themes (and a theme can have way more functionality than just providing the look for the website (a massive mistake in my book). I think that's one reason you see many base themes being submitted - hardcore developers like working on these type things (for free in their spare time - though this particular response is being written during my company's weekly contrib time (thanks Annertech)), and then they in turn make the job of creating custom themes easier for people like me/Daniel/etc as we can leverage from the work of the base theme.

I think you are going to see less contributed themes for Drupal 8 than there were for Drupal 7.

I'm going to leave that discussion there, and not continue it, as this thread is about getting Theme Breeze promoted to full project status. Thanks for the apology.

templates/comment.html.twig has incorrect indenting for the footer and div sections, i.e. lines 77 through 104 are indented two spaces too many. Can I ask what you used to notice this?

My eyes :-)

A few quick things:

• The js/script.js file is still in the repository.
• If you've removed the tinynav then you should update the README.md file as it still mentions it.
• Would it be possible to rework the codebase so that the Bootstrap library could be downloaded and used as-is, instead of copying individual files? This could be handled as a follow-up, but I think it'd make life easier for people trying to use the theme.

Thanks for your contribution, Daniel!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or 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

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. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.