Problem/Motivation

Remove non essential classes from core - the classes are in classy, cores templates should be as clean as possible.
js required classes should be prefixed to js- according to code standards
simpletest should be done with classy theme, not stark

Proposed resolution

Twig Templates to modify

  • core/modules/system/templates/page.html.twig
  • core/modules/system/templates/pager.html.twig
  • core/modules/system/templates/progress-bar.html.twig

Remaining tasks

User interface changes

See parent issue(s) and related issue(s) for details,

Files: 
CommentFileSizeAuthor
#21 p-templates-5.diff10.3 KBvalderama
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,706 pass(es), 5 fail(s), and 0 exception(s). View
#20 p-templates-4.diff9.61 KBvalderama
#16 p-templates-3.diff9.01 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,797 pass(es). View
#11 progressbar-before.png42.52 KBrteijeiro
#11 progressbar-after.png43.58 KBrteijeiro
#10 p-templates-2.diff9.57 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,817 pass(es). View
#10 Installing_Drupal___Drupal.png509.67 KBmortendk
#4 clean-p-templates.diff6.04 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,228 pass(es). View
#1 issue-2407737-1.patch16.72 KBSivaji
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,902 pass(es). View

Comments

Sivaji’s picture

Status: Active » Needs review
FileSize
16.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,902 pass(es). View
davidhernandez’s picture

Status: Needs review » Postponed

Postponing this until we decide how we are moving forward. Keep an eye on #2348543: [meta] Consensus Banana Phase 2, transition templates to the Classy theme for updates. Thanks.

davidhernandez’s picture

Title: Copy system templates p*.html.twig to Classy » Remove classes from system templates p*.html.twig
Issue summary: View changes
Status: Postponed » Needs work

Un-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.

mortendk’s picture

Status: Needs work » Needs review
FileSize
6.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,228 pass(es). View

progress-bar.html.twig have a lot of tie ins with core/misc/progress.js
must be looked into renaming classes there to js- progress__label etc

pager.html.twig & page.html.twig should be clean

mortendk’s picture

Issue summary: View changes
Issue tags: +classy
mortendk’s picture

Status: Needs review » Needs work
Issue tags: +drupalcafe-feb2015

pregressbar css needs to be renamed & tested

Xen’s picture

Assigned: Unassigned » Xen

Grabbing.

Xen’s picture

Assigned: Xen » Unassigned

Arg.. Ungrabbing.

mortendk’s picture

mortendk’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
509.67 KB
9.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,817 pass(es). View

prefixed the js needed classes with .js- and took screenshots

rteijeiro’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
43.58 KB
42.52 KB

Great job @mortendk!

Progressbar BEFORE

Progressbar AFTER

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Something is feeling really wrong about prefixing everything that javascript touches with js- especially when the class is being used to give the element style.

davidhernandez’s picture

Status: Needs review » Needs work

Agreeing with Alex. I don't think js- classes should show up in CSS files, and most of the changes in this patch are CSS. The point of the prefix is to highlight classes that are only needed for javascript. If we use them for both, it is no different than using the non-js classes for style and javascript. I'm thinking add separate js- classes or rework the javascript.

mortendk’s picture

yeah looks like we also have to rewrite all of that
Well now at least we know where this body is burried

mortendk’s picture

mortendk’s picture

Status: Needs work » Needs review
FileSize
9.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,797 pass(es). View

ok i have taken same approach with the js- prefix as in forms & indentation etc.
js- prefix classes added in for js operations

alexpott’s picture

We need to update the CR

mortendk’s picture

Issue tags: +Needs screenshots

updated with the changes in the css classes, whats added in as js- prefixes & which classes we have removed in core, but is in classy

Do we want to fix the css MAT seperation here as well - and screenshots since we are changing js selectors ?

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/misc/progress.js
    @@ -21,10 +21,10 @@
    +    this.element.html('<div class="js-progress__label progress__label">&nbsp;</div>' +
    +      '<div class="progress__track"><div class="js-progress__bar progress__bar"></div></div>' +
    +      '<div class="js-progress__percentage progress__percentage"></div>' +
    +      '<div class="js-progress__description progress__description">&nbsp;</div>');
       };
    

    Maybe it would be nice to do #2452381: Use Drupal.theme for progress.js

  2. +++ b/core/modules/system/templates/pager.html.twig
    @@ -32,12 +32,12 @@
    -  <nav class="pager" role="navigation" aria-labelledby="pagination-heading">
    -    <h4 id="pagination-heading" class="visually-hidden">{{ 'Pagination'|t }}</h4>
    -    <ul class="pager__items">
    +  <nav role="navigation" aria-labelledby="pagination-heading">
    +    <h4 class="visually-hidden">{{ 'Pagination'|t }}</h4>
    +    <ul>
    

    So, I see

        this.$view.find('ul.pager__items > li > a, th.views-field a, .attachment .views-summary a')
          .each(jQuery.proxy(this.attachPagerLinkAjax, this));

    in core/modules/views/js/ajax_view.js:105 ... doesn't this break things?

valderama’s picture

FileSize
9.61 KB

Hi - found a reference to this issue on twitter, and thought I could do a quick patch.

I have taken on dawehners second point from the last comment and added a class "js-pager__items" to the ul-tag - and changed the jQuery selector in ajax_views.js accordingly. That should keep this part working. Everything else is the same as in the last patch.

And a little side note: Keeping the empty div here seems strong. Maybe a more semantic tag would be better? It is not a big deal however...

+++ b/core/modules/system/templates/page.html.twig
@@ -70,21 +70,21 @@
     {% if site_name or site_slogan %}
-      <div class="name-and-slogan">
+      <div>
valderama’s picture

Status: Needs work » Needs review
FileSize
10.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,706 pass(es), 5 fail(s), and 0 exception(s). View

I forgot to add the "js-pager__items" class also to the classy version of pager.html.twig. Included in the new patch.

I have tested the ajax pager now in bartik, seven and stark - and it works now in classy-themes (bartik and seven) and in non-classy themes (stark).

Status: Needs review » Needs work

The last submitted patch, 21: p-templates-5.diff, failed testing.

valderama’s picture

I guess the PagerTest fails because it looks for li.pager__items - which are not present in non-classy themes. Same for AggregatorRenderingTest.

So the patch fails because simpletest uses a non-classy theme?

mortendk’s picture

bumb

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.