Problem/Motivation

Split from #2880237: [meta] Refactor system/base library.

This class/CSS was added in 2014 as part of a Seven tabs redesign. It is only used for Claro local tasks, but the CSS is loaded on every request to every Drupal site.

We can move the CSS to its own library, then attach it from the Claro vertical tabs template.

Steps to reproduce

Install standard.

Go to a page like admin/structure/types/manage/article/fields

Resize the browser down to a narrow viewport so you get Claro's hamburger menu for the local tasks.

Make sure the hamburger menu doesn't have unexpected outline button theming - the reset-appearance CSS/class is what removes that.

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#23 3512404-nr-bot.txt91 bytesneeds-review-queue-bot
#19 3512404-nr-bot.txt91 bytesneeds-review-queue-bot
#8 3512404-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3512404

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:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !11447

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

haven't manually test but all tests appear to be failing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased again.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Following the steps in the issue summary confirmed everything is behaving as normally.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3512404-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

dcam made their first commit to this issue’s fork.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

It didn't really need a rebase. Not sure why it thought the MR didn't apply.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

can't apply as a patch or rebase the MR from the UI, need some manual rebase/merge

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Rebased again.

dcam’s picture

dcam commented

@catch Sorry I hadn't finished the rebase yet. I was busy with work.

catch’s picture

catch
he/him
Primary language English
commented

Ahh apologies from me too, I missed that you were already doing the rebase (although I think I ended up rebasing on top of your in-progress rebase).

These are one of few types of MRs I don't mind rebasing because it's nearly always because the numbers are going down :)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Is this really useful anywhere else? Can we just deprecate it and add the relevant parts directly to Claro's CSS for .tabs__trigger?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

@longwave we can't deprecate a files only a library, so that could happen in a follow-up, but not really here, unless we remove it without deprecation.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

By moving it to its own library anyone who was using it has to make changes by attaching that library, right?

So if they have to make changes anyway maybe we just remove it instead, because this feels kinda useless on its own.

catch’s picture

catch
he/him
Primary language English
commented

#17 that's pretty accurate. If we remove it outright, we'd need to do the following:

1. Copy the CSS to somewhere in stable9 that's always loaded. stable9 doesn't have a catch-all file, only overrides, so either need to pick an existing file that's not a perfect match or add a legacy.css or something.

2. Add the CSS to Claro's tabs CSS somewhere.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3512404-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

I'm not sure why it thought the MR no longer applies. It was fine. Restoring RTBC.

dcam’s picture

dcam commented
Status: Reviewed & tested by the community » Needs work

It actually needed a rebase today. But now the tests are failing due to deprecations with the following warning...

Sorry, I messed up the rebase. I'm trying to fix it.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

I restored the original changes from the MR properly. Sorry about that everyone.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3512404-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

It didn't actually need a rebase, but I did one anyway just to be certain. Restoring RTBC.

dcam’s picture

dcam commented

Since one of the other library split issues got committed this one needed a rebase with updated performance metrics. I also updated the deprecation version, but I'm not going to roll back the RTBC status for that, especially since @alexpott allowed self-RTBCing in the other issue for the same minor update.

This issue can be added to the CR I made yesterday for the 11.3.0 splits: #3530832: system/base split into more conditionally loaded libraries.

nod_ closed merge request !11447

  • nod_ committed f38f5f45 on 11.x 
    Issue #3512404 by catch, dcam, smustgrave: Move reset-appearance.module....
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed f38f5f4 and pushed to 11.x. Thanks!

updated the CR too, thanks!

nod_’s picture

nod_
Primary language French
Location Lille
commented

Status: Fixed » Closed (fixed)

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