Move classes out of the preprocess functions and into the Twig templates. Use the addClass() attribute method to add classes in the template. Use the clean_class filter to filter class names, if necessary. Maintain all existing functionality and ensure all existing class names are still in the markup, even ones that are inherited.

See the following issues for more detailed examples:
#2217731: Move field classes out of preprocess and into templates
#2254153: Move node classes out of preprocess and into templates

See this change record for information about using the addClass() method:
https://www.drupal.org/node/2315471

See this change record for more information about the phase 1 process of moving class from preprocess to templates:
https://www.drupal.org/node/2325067

Preprocess Functions Modified

template_preprocess_ckeditor_settings_toolbar

Twig Templates Modified

ckeditor-settings-toolbar.html.twig

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it improves the Themer Experience.
Issue priority Normal.
Unfrozen changes Unfrozen because it changes markup.
Prioritized changes The main goal of this issue is usability of the new Twig theming layer, and since it adjusts markup it must be completed prior to RC.
Disruption This issue is a prioritized change (theme improvements) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

Issue tags: +FUDK
star-szr’s picture

star-szr’s picture

Status: Postponed » Active

#2306515: SafeMarkup in CKEditor toolbar configuration UI — fix by not generating markup in PHP but using Twig Macros is no longer a major bug so un-postponing, we just talked about this on the Twig call. Whatever gets in first gets in first, I think either way we should get a patch up here.

davidhernandez’s picture

I'm good with un-postponing. I only see a couple classes in preprocess. It shouldn't be terribly difficult to move, and then we can definitely close out phase 1 completely.

sidharthap’s picture

Status: Active » Needs review
FileSize
1020 bytes

Here is the first attempt. Please correct me, If it is wrong.

Status: Needs review » Needs work

The last submitted patch, 5: 2329781-ckeditor-toolbar-5.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

@sidharthap, what we are trying to do here is actually remove the class completely from the preprocess function and put it into the template file.

I'm uploading a patch that I think gets the right classes. The only weirdness is that the 'multiple' buttons seem to only be the separators so they don't get the normal button class. That is why I had to put some logic in there.

I didn't try to remove the class on the icon span tag. We would have to do some serious re-arranging I think. Buuuuuuuut, the "#2306515: SafeMarkup in CKEditor toolbar configuration UI..." issue does all of that, so I don't think that is a problem we need to solve here.

davidhernandez’s picture

For anyone reviewing, this template shows up on the admin page, admin/config/content/formats/manage/full_html, not on the node edit page where the wysiwyg editor actually appears.

lauriii’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

It doesn't break the functionality and the classes seem to be the same after applying the patch.

I also added the beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a02e778 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed a02e778 on 8.0.x
    Issue #2329781 by davidhernandez, sidharthap: Move CKEditor toolbar...

Status: Fixed » Closed (fixed)

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