Move the bc layer for views UI CSS classes from views to stable9

Created on 6 June 2018, over 6 years ago
Updated 8 March 2024, 8 months ago

Follow-up to #2849954: Double underscores are not preserved in main views CSS classes defined in views UI

Problem/Motivation

If set "my__class" value via Views UI (Edit View > Advanced > CSS) this will give two classes at the html-output:

my__class
my--class

Proposed resolution

Remove the BC layer
Provide a Change record with instructions how to restore the BC classes if they are needed

📌 Task
Status

Fixed

Version

11.0 🔥

Component
Views 

Last updated about 1 hour ago

Created by

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇳🇱Netherlands Lendude Amsterdam

    This still being in, is now leading to people opening bugs 🐛 Views CSS class is changed when underscores and dashes are mixed Closed: duplicate , lets see if we can get rid of this

  • Merge request !5776Remove the BC layer for css classes → (Open) created by Lendude
  • Status changed to Needs review 11 months ago
  • 🇳🇱Netherlands Lendude Amsterdam

    So this just removes the BC layer. I don't see any way to detect that people are using these classes, all we can detect is if we are suppling them with BC classes, so raising a deprecation notice feel weird since it might not be relevant at all.

    Would it maybe be sufficient to add a CR with a code snippet that could recreate the BC classes if somebody still needs them?

    Something like this seems to do the trick:

    function mytheme_preprocess_views_view(&$variables) {
      if (!empty($variables['attributes']['class'])) {
        $bc_classes = preg_replace('/[^a-zA-Z0-9- ]/', '-', $variables['attributes']['class']);
        $variables['attributes']['class'] = array_merge($variables['attributes']['class'], $bc_classes);
      }
      if (!empty($variables['css_class'])) {
        $existing_classes = explode(' ', $variables['css_class']);
        $bc_classes = preg_replace('/[^a-zA-Z0-9- ]/', '-', $existing_classes);
        $variables['css_class'] = implode(' ', array_merge($existing_classes, $bc_classes));
      }
    }
    
  • Pipeline finished with Success
    11 months ago
    Total: 3004s
    #62569
  • Status changed to Needs work 11 months ago
  • 🇺🇸United States smustgrave

    Could the issue summary be updated with the proposed solution, "Leave two classes as is." doesn't seem correct anymore.

    And you're right not sure how to BC a class removal, and not the first ticket I've seen around this. A CR may be the best thing but even then I imagine if someone is using those removed classes they'll notice when something breaks before the CR.

  • Status changed to Needs review 11 months ago
  • 🇳🇱Netherlands Lendude Amsterdam

    Updated the IS with the new proposal on how to address this.

    I'll ping somebody to see if we can get some more opinions on how to deal with HTML class removal.

  • 🇺🇸United States smustgrave

    Whatever is decided should post here too 📌 Remove .js class from core Needs work

  • 🇫🇮Finland lauriii Finland

    Can we move the BC class to Stable 9? That's the standard approach and allows us to get rid of it once we replace Stable 9.

  • 🇺🇸United States smustgrave

    This may be a dumb question but since the classes are dynamic how do you add those to just stable?

  • 🇫🇮Finland lauriii Finland

    Couldn't we just move the logic from template_preprocess_views_view to stable9_preprocess_views_view?

  • Status changed to Needs work 11 months ago
  • 🇳🇱Netherlands Lendude Amsterdam

    Yeah that should work, just add that snippet in #17 to stable9

  • Status changed to Needs review 11 months ago
  • 🇳🇱Netherlands Lendude Amsterdam

    Did what @lauriii suggested. The other case in stable9.theme that seems to have done this didn't get test coverage as far as I can tell, so no test coverage for that for now

  • Pipeline finished with Success
    11 months ago
    Total: 749s
    #63002
  • Status changed to RTBC 11 months ago
  • 🇺🇸United States smustgrave

    Change looks good.

    So if this the path forward for any other class removals?

  • Status changed to Needs work 11 months ago
  • 🇬🇧United Kingdom catch

    This looks good but we need a change record.

    Something like 'Views used to duplicate custom classes like my__class added in the views UI into an additional my--classas a backwards compatibility layer. This backwards compatibility layer has now been moved to the Stable9 theme. If you were relying on these classes, you should either update your CSS to target the correct class, or add Stable9 as a dependency of your theme'

  • 🇬🇧United Kingdom catch
  • Status changed to RTBC 11 months ago
  • 🇺🇸United States smustgrave

    Created a CR from #27.

    Made the recommendation to target the correct CSS classes. Do we know how long stable9 will be around? If that should be added to the CR.

  • 🇳🇿New Zealand quietone

    I'm triaging RTBC issues . I read the IS and the comments. I didn't find any unanswered questions or other work to do. Although, the proposed resolution is a bit out of date.

    Leaving at RTBC.

  • First commit to issue fork.
    • nod_ committed 3923a9f8 on 11.x
      Issue #2977950 by Lendude, smustgrave, lauriii: Move the bc layer for...
  • Status changed to Fixed 9 months ago
  • 🇫🇷France nod_ Lille

    Committed 3923a9f and pushed to 11.x. Thanks!

    Not backporting, so that it'll only be released with D11.

  • Pipeline finished with Success
    9 months ago
    Total: 518s
    #102234
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024