Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter BEM function classes to conform to CSS spec #10

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

codechefmarc
Copy link
Collaborator

@codechefmarc codechefmarc commented Sep 29, 2023

Summary

This PR fixes/implements the following bugs/features

  • Filters classes passed to the bem function to conform to the CSS class specifications listed here: https://www.w3.org/TR/CSS21/syndata.html#characters
  • This filter will replace invalid characters with spaces. For example: a class of my^selector would output my selector as the class names.
  • It is not likely, but there are possibly breaking changes if class names were using invalid characters that were expected in the output.

Explain the motivation for making this change. What existing problem does the pull request solve?

Class names should conform to the CSS specification and this function is outputting generated classes. Each of these generated classes should behave within the spec.

Documentation update (required)

Documentation should be added for the bem function.

How to review this pull request

  • Start Storybook instance, npm run storybook
  • Edit a component twig file, such as a basic text field component
  • When using the bem function, pass in some class names that are and aren't valid, for example:
{% set text_field__base_class = text_field__base_class|default('text-field') %}
{% set text_field__extra_classes = ['good-class-name', 'with^carrot', 'with_underscore', 'with%percent', 'with()parens'] %}
{% set text_field__attributes = {
  class: bem(text_field__base_class, text_field__modifiers, text_field__blockname, text_field__extra_classes),
} %}

<div {{ add_attributes(text_field__attributes) }}>
  {% block prefix_suffix %}
  {% endblock %}
  <div {{ bem('inner', [], text_field__base_class) }}>
    {% include "@atoms/typography/text/yds-text.twig" with {
      text__content: text_field__content,
    } %}
  </div>
</div>
  • Verify that the output in the DOM has replaced the ^, %, and () with spaces.
  • Verify that the good-class-name and with_underscore are unmodified.
  • Change the base class in the example above to 2text-field and verify that the 2 gets stripped out.

┆Issue is synchronized with this Clickup task by Unito

// Helper function to sanitize and validate class names
function sanitizeClassName(className) {
// Strips out initial numbers. Replace non-matches with spaces, does strip non-english characters
const sanitized = className.match(/-?[_a-zA-Z]+[_a-zA-Z0-9-]*/g);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codechefmarc I think the approach for this is to take it to a Engineering Team meeting and get some other eyes on this before merging. I remember we had 3 options for the regex here, I don't remember if this is the one we landed on or not but seeing as it is a breaking change I would make sure we bump the major version and advertise that this version could break class names and thus the look and feel of a site for sites that used characters that are no longer supported.

@codechefmarc codechefmarc merged commit 714e531 into master Jul 31, 2024
1 check passed
@codechefmarc codechefmarc deleted the bem-filters branch July 31, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants