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

Issue fork drupal-3436855

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

catch created an issue. See original summary.

catch’s picture

Status: Active » Needs review

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

smustgrave’s picture

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

catch’s picture

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?

I don't usually touch CSS, deleting it is about my limit, so not sure what the process is here.

longwave’s picture

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 unless this is a hardcoded list in Views already? Anyway I think it's better to fix the problem at source, permanently and more specifically.

Thinking about this and #3436478: Use regex instead of DOM parsing in BigPipe::getPlaceholderOrder() 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.

smustgrave’s picture

#2935897: [policy, then docs] Change how we deprecate classes not sure if this follows under this policy

catch’s picture

Status: Needs review » Needs work

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.

catch’s picture

smustgrave’s picture

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

longwave’s picture

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.

catch’s picture

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.

catch’s picture

@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?

longwave’s picture

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.

catch’s picture

Status: Needs work » Needs review

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.

smustgrave’s picture

Status: Needs review » Needs work

Left a comment on MR.

catch’s picture

Status: Needs work » Needs review

Responded on the MR.

smustgrave’s picture

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Added a very very small comment.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

catch’s picture

Status: Needs work » Needs review
Parent issue: » #2880237: [meta] Refactor system/base library

Added deprecation support and rebased.

Adding #2880237: [meta] Refactor system/base library 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.

catch’s picture

Modern CSS uses start and end instead of left and right for RTL support. I opened #3515050: Align CSS rules don't take into account RTL langaguages and are often unused 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.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs upgrade path tests

Can we add post_update test coverage

catch’s picture

We can but I'm starting to question whether we shouldn't do this in one step with #3515050: Align CSS rules don't take into account RTL langaguages and are often unused 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.

smustgrave’s picture

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

catch’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path tests

Given #3515050: Align CSS rules don't take into account RTL langaguages and are often unused I've gone back to the original option here, we can open a follow-up to completely rename everything and add an upgrade path. The entirety of views.module.css needs to go, not just these three rules, but one thing at a time.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.58 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Seems maybe changes views.theme.inc. got pulled back in.

catch’s picture

Status: Needs work » Needs review

Oops bad merge conflict - fixed.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

LGTM

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 4eafbc51fd2 to main and cfd0965249c to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed c266f0f9 on 11.x
    refactor: #3436855 Remove the views-align-* CSS classes
    
    By: catch
    By:...

  • alexpott committed 092d4256 on main
    refactor: #3436855 Remove the views-align-* CSS classes
    
    By: catch
    By:...

  • alexpott committed db73710e on 11.x
    fix: Performance tests broke due to revert and #3436855 Remove the views...

  • alexpott committed c0ceb346 on main
    fix: Performance tests broke due to revert and #3436855 Remove the views...

Status: Fixed » Closed (fixed)

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