Problem/Motivation

It's common to provide styling for base elements, elements that don't have classes. This styling is used in situations where a user can enter formatted HTML text. This can often introduce conflicts with UI components, if they are using the elements that have styling applied.

Some themes get around this by prefixing the base selectors with a wrapper class a indicates they are free text, not a UI component.

Bartik has an attempt to do this:

.region-content ul,
.region-content ol {
  margin: 1em 0;
  padding: 0 0 0.25em 15px; /* LTR */
}

The logic of this selector is flawed because anything can go inside the content region, not just free text.

Proposed resolution

Add a class in Classy's field.html.twig that indicates the field is formatted HTML text: formatted-text

Remaining tasks

Class name consensus
Patch

User interface changes

None

API changes

None

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it's a handy hook for themers
Issue priority Not critical because it's a nice to have
Unfrozen changes Unfrozen because it only changes markup

Comments

jeni_dc’s picture

Status: Active » Needs review
StatusFileSize
new920 bytes

As it turns out I was overcomplicating this issue a bit at DrupalCamp North. Classy already contains a field--text.html.twig file that is used by all formatted text fields. That file is adding a "clearfix" class to all formatted text fields, so I just added an additional 'text-formatted' class there. Now all formatted text fields will get both classes. That template does not get used for any plain text or list fields.

I've attached a patch with the new class added, that also contains comments about why that class was added and the URL back to this issue, similar to the comments for the clearfix class.

lewisnyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new998.76 KB

Great! That's a nice comment. I didn't realise we were already targeting formatted text fields. I added all text fields and added some dummy styling to show it applies correctly.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Shouldn't we also be fixing the Bartik example in the issue summary?

jeni_dc’s picture

I'm more than happy to fix the issue in Bartik as well, but since Bartik depends on Classy, wouldn't this need to be committed first?

alexpott’s picture

Status: Needs review » Needs work

@jeni_dc it can be fixed in the same patch

lewisnyman’s picture

Status: Needs work » Reviewed & tested by the community

I discussed this with Emma. Because the ul/ol styling in Bartik is a hot mess, changing the selector in Bartik in this issue will open up a high risk of regression. See: #2509902: Remove block.css list styles and add the code to other components.

This issue is for Classy, which means anyone sub-theming classy should benefit from this. My preference is to keep it that way instead of turning it into a problematic Bartik issue, otherwise we run the risk of this never getting in before RC and then people can't benefit from this class.

I've created a followup here: #2541252: Replace the .region-content ul/ol selector with text-formatted to refactor code + fix visual bugs

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed dead5be and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed dead5be on 8.0.x
    Issue #2539860 by jeni_dc, LewisNyman: Add a class to field that contain...

Status: Fixed » Closed (fixed)

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