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

Proposed resolution

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

Twig Templates to modify

  • core/modules/system/templates/select.html.twig
  • core/modules/system/templates/status-messages.html.twig
  • core/modules/system/templates/status-report.html.twig
  • core/modules/system/templates/system-admin-index.html.twig
  • core/modules/system/templates/system-config-form.html.twig
  • core/modules/system/templates/system-themes-page.html.twig

Do not modify templates with a strike through their name. Those were not copied to Classy.

Remaining tasks

User interface changes

API changes

Original report by @username

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sivaji_Ganesh_Jojodae’s picture

Status: Active » Needs review
FileSize
12.79 KB
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 starterkit theme for updates. Thanks.

davidhernandez’s picture

Title: Copy system templates s*.html.twig to Classy » Remove classes from system templates s*.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
1.24 KB

Status: Needs review » Needs work

The last submitted patch, 4: cealn-template-s.diff, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

MessageTest needs to specify Classy.

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue tags: +drupalcafe-feb2015
danquah’s picture

Assigned: Unassigned » danquah
danquah’s picture

Did one minor change so that we now pull out the class attribute explicitly and render it, instead of the entire Attributes object.

Should we want to actually allow the entire Attributes object to be rendered we should make sure our own attributes does not collide (eg. "role").

Screenshot of the resulting markup attached, notice that the current template_preprocess_status_messages() does not add a "attributes" - so if someone wants to add classes they need to implement their own preprocess.

danquah’s picture

Assigned: danquah » Unassigned

Status: Needs review » Needs work

The last submitted patch, 10: remove_classes_from-2407743-10.patch, failed testing.

joelpittet’s picture

Thanks it's looking pretty good, just a couple of nitpicks:

  1. +++ b/core/modules/system/src/Tests/Theme/MessageTest.php
    @@ -25,6 +25,10 @@ class MessageTest extends KernelTestBase {
    +    ¶
    

    Got an extra tab or space here by accdient.

  2. +++ b/core/modules/system/templates/status-messages.html.twig
    @@ -52,6 +47,4 @@
    -  {# Remove type specific classes. #}
    -  {{ attributes.removeClass(classes) }}
    

    That's kinda a weird place for this anyways, +1 for removing that.

  3. +++ b/core/modules/system/templates/status-messages.html.twig
    @@ -26,13 +26,8 @@
    +
    

    No need for this extra line break.

mortendk’s picture

Status: Needs work » Needs review
FileSize
2.7 KB

fixed the spaces

mortendk’s picture

FileSize
7.71 KB

ups breadcrump template sneaked into #15

mortendk’s picture

FileSize
2.06 KB

ya gotte be kidding me ... right patch this time the S templates

The last submitted patch, 16: cleanup-templates-f-2.diff, failed testing.

mortendk’s picture

... goes make coffe interdiff added & patch cleaned up

The last submitted patch, 17: clean-templates-s.patch, failed testing.

rteijeiro’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
85.43 KB
59.98 KB
61.96 KB

I think it still needs work. In Stark it status messages seem to lose the styling. In Bartik is correct. Check screenshots:

Stark BEFORE

STARK AFTER

BARTIK

mortendk’s picture

Status: Needs work » Needs review

it works as it should - its by design that classes are removed (see the banana #2289511: [meta] Results of Drupalcon Austin's Consensus Banana & https://www.drupal.org/node/2348543 )
Classes should not be there in stark unless they are needed for the module to work - a color is not required to work ;)
themers that goes with stark as basetheme, knows what they are doing and will have to provide classes that they need.

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Oh, thanks! That's all I need to RTBC this!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: clean-template-s-2.patch, failed testing.

mortendk queued 19: clean-template-s-2.patch for re-testing.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Template changes are not frozen in beta. Committed 00df5ba and pushed to 8.0.x. Thanks!

  • alexpott committed 00df5ba on 8.0.x
    Issue #2407743 by mortendk, danquah, rteijeiro, davidhernandez, sivaji@...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.