Remove the views-align-* CSS classes

Created on 28 March 2024, about 1 year ago

Problem/Motivation

views.module.css includes the following rules:

/* table style column align */
.views-align-left {
  text-align: left;
}
.views-align-right {
  text-align: right;
}
.views-align-center {
  text-align: center;
}

system.align.css has the following:

.text-align-left {
  text-align: left;
}
.text-align-right {
  text-align: right;
}
.text-align-center {
  text-align: center;
}
.text-align-justify {
  text-align: justify;
}

Spot the difference.

Steps to reproduce

Proposed resolution

Use the core classes instead.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component
ViewsΒ  β†’

Last updated about 1 hour ago

Created by

πŸ‡¬πŸ‡§United Kingdom catch

Live updates comments and jobs are added and updated live.
  • CSS

    It involves the content or handling of Cascading Style Sheets.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • Status changed to Needs review about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Maybe we can do this. If not, we need to change the the tables plugin and add an upgrade path.

  • Merge request !7235What about this. β†’ (Open) created by catch
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Could we put a small comment for the str_replace just so we know why?

  • πŸ‡¬πŸ‡§United Kingdom catch

    Added a comment, but I'm not sure this approach will actually get committed - we might need to go the full config upgrade path/hook_view_presave() route to rename the options instead.

    Also not at all sure what this means for stable9 - is it supposed to add back the old classes or something?

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Hmm this feels a bit hacky - surely there is a chance of false positives here and debugging this is going to be tricky for some. I think it's better to fix the problem at source, permanently and more specifically.

    Thinking about this and πŸ“Œ Use regex instead of DOM parsing in BigPipe::getPlaceholderOrder() RTBC also gave me an idea of how we could deprecate CSS classes: an event subscriber that parses the entire response (DOM parsing or maybe regex if we're feeling lucky) and issues deprecations if they are found in the output? Not perfect, but not sure what other options we have.

  • Pipeline finished with Canceled
    about 1 year ago
    Total: 236s
    #132007
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    πŸ“Œ [policy, then docs] Change how we deprecate classes Fixed not sure if this follows under this policy

  • Status changed to Needs work about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Pushed a commit to get rid of the str_replace() and rename the options - but no upgrade path for that yet.

    also gave me an idea of how we could deprecate CSS classes: an event subscriber that parses the entire response (DOM parsing or maybe regex if we're feeling lucky) and issues deprecations if they are found in the output?

    Would probably need to be toggled like Twig debug because nothing stops themes implementing CSS for those classes, and adding them back via preprocess/templates.

  • Pipeline finished with Success
    about 1 year ago
    Total: 501s
    #132012
  • πŸ‡¬πŸ‡§United Kingdom catch

    @smustgrave πŸ“Œ [policy, then docs] Change how we deprecate classes Fixed is for PHP classes.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Oops feel like I've seen a policy ticket around this topic though.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    So even with removing the classes from the table options it's quite possible someone is using these handy classes elsewhere in the custom class fields in Views, or in a template, because they've been around forever and it saves you making your own. If we just delete them from CSS we risk breaking layouts, because we didn't tell people not to use them.

  • πŸ‡¬πŸ‡§United Kingdom catch

    unless this is a hardcoded list in Views already?

    It is a hard-coded list in views already, it looks like it's generic, but it's fake-genericism - there are just three classes used in Tables.php and the preprocess only runs on tables.

    I also wonder whether this is even a legit feature, seems like explicit, hard-coded text alignment doesn't fit with RTL languages. I guess it's OK if you leave it unconfigured, and it'll mostly be on custom views, but it would be tempting to offer only nothing (which defaults to left or right alignment depending on language direction) and center. That'd be an even bigger change though.

  • πŸ‡¬πŸ‡§United Kingdom catch

    @longwave

    If we just delete them from CSS we risk breaking layouts, because we didn't tell people not to use them.

    Isn't that what core/themes/stable9/css/views/views.module.css is for?

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Oh yes, I forgot about that. Wonder if we should try and do more of these before Drupal 11 and make a leaner and cleaner stable11 theme or if it's too late for that.

  • Pipeline finished with Failed
    about 1 year ago
    Total: 179s
    #132061
  • Pipeline finished with Failed
    about 1 year ago
    Total: 365s
    #132066
  • Pipeline finished with Success
    about 1 year ago
    Total: 552s
    #132276
  • Status changed to Needs review about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Added an upgrade path.

    Having done that though, I wondered if we should make the config keys 'left', 'center', 'right' and then expand those to the class names in preprocess, that way if core ever updates these classes, we wouldn't need another update path. So went ahead and did that too.

  • Pipeline finished with Success
    about 1 year ago
    Total: 502s
    #132300
  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Left a comment on MR.

  • Status changed to Needs review about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Responded on the MR.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Can a comment be added to stable9 then so we know why that’s needed there.

  • Status changed to RTBC about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Added a very very small comment.

  • Pipeline finished with Success
    about 1 year ago
    Total: 655s
    #133492
  • Status changed to Needs work about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    I think we need to go through the deprecation process so that module maintainers get information to update any views provided by modules.

  • Pipeline finished with Failed
    9 days ago
    Total: 114s
    #456217
  • Status changed to Needs review 9 days ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Added deprecation support and rebased.

    Adding πŸ“Œ Refactor system/base library Needs work as parent issue because while this obviously is not part of the system/base library, consolidating these classes means they no longer duplicate system/base's align classes (which currently are not affected by that meta issue because they're quite complicated to factor out to a library.

  • Pipeline finished with Success
    9 days ago
    Total: 337s
    #456224
  • Pipeline finished with Success
    9 days ago
    Total: 523s
    #456248
  • πŸ‡¬πŸ‡§United Kingdom catch

    Modern CSS uses start and end instead of left and right for RTL support. I opened πŸ“Œ Views table column align options don't take into account RTL langaguages - remove? Active to discuss the lack of RTL support and whether we even want to maintain this feature in views at all. But I think within the scope of de-duplicating the issues this issue is probably OK.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Can we add post_update test coverage

  • πŸ‡¬πŸ‡§United Kingdom catch

    We can but I'm starting to question whether we shouldn't do this in one step with πŸ“Œ Views table column align options don't take into account RTL langaguages - remove? Active instead because adding two sets of upgrade paths with their own test coverage is not ideal.

    Or if we might do that issue, or rename the classes again to match logical CSS rules or similar, go back to the original approach here which didn't require an upgrade path as quick fix and revisit the approach in a follow-up.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Less steps the better I think as this could potentially break something, edge cases

Production build 0.71.5 2024