Refactor system/base library

Created on 22 May 2017, almost 8 years ago
Updated 21 January 2023, about 2 years ago

Currently, system_page_attachments() ads the system/baselibrary to all pages and this includes all kind of styles that might not be required for the page. I think that the library should be split into several smaller libraries.

Styles that are specific to certain pages, should be attached only on those pages.

For example the css/components/system-status-counter.css file is only required for the status-report-counter.html.twig template, and it makes sense to move this CSS file to a separate library one for the template or one for the system status page, toghether with other files like css/components/system-status-report-counters.css and css/components/system-status-report-general-info.css

And maybe css/components/ajax-progress.module.css should be moved to the system/drupal.ajax library.

Here is the definition of the library, and you can see that there are many other files that probably should be moved to a more specific library, like "tabledrag.module.css", "tablesort.module.css" or "autocomplete-loading.module.css"

base:
  version: VERSION
  css:
    # Adjust the weights to load these early.
    component:
      css/components/ajax-progress.module.css: { weight: -10 }
      css/components/align.module.css: { weight: -10 }
      css/components/autocomplete-loading.module.css: { weight: -10 }
      css/components/fieldgroup.module.css: { weight: -10 }
      css/components/container-inline.module.css: { weight: -10 }
      css/components/clearfix.module.css: { weight: -10 }
      css/components/details.module.css: { weight: -10 }
      css/components/hidden.module.css: { weight: -10 }
      css/components/item-list.module.css: { weight: -10 }
      css/components/js.module.css: { weight: -10 }
      css/components/nowrap.module.css: { weight: -10 }
      css/components/position-container.module.css: { weight: -10 }
      css/components/progress.module.css: { weight: -10 }
      css/components/reset-appearance.module.css: { weight: -10 }
      css/components/resize.module.css: { weight: -10 }
      css/components/sticky-header.module.css: { weight: -10 }
      css/components/system-status-counter.css: { weight: -10 }
      css/components/system-status-report-counters.css: { weight: -10 }
      css/components/system-status-report-general-info.css: { weight: -10 }
      css/components/tabledrag.module.css: { weight: -10 }
      css/components/tablesort.module.css: { weight: -10 }
      css/components/tree-child.module.css: { weight: -10 }
โœจ Feature request
Status

Needs work

Version

10.1 โœจ

Component
CSSย  โ†’

Last updated about 10 hours 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.

  • @xamount opened merge request.
  • Status changed to Needs review about 2 years ago
  • ๐Ÿ‡น๐Ÿ‡นTrinidad and Tobago xamount

    I took a stab at it and refactored most of the css that should be refactored. The remainder of css were not specific to any templates and/or were too broad and used in multiple areas of Drupal.

  • ๐Ÿ‡น๐Ÿ‡นTrinidad and Tobago xamount

    One important thing I noticed is that some themes (core themes included) are using libraries-override to override some of the css in the system/base library. I don't think it will break anything, but the css it will try to find will not exist.

    For e.g. in claro.info.yml:

    libraries-override:
      system/base:
        css:
          component:
            css/components/ajax-progress.module.css: css/components/ajax-progress.module.css
            css/components/autocomplete-loading.module.css: css/components/autocomplete-loading.module.css
            css/components/system-status-counter.css: css/components/system-status-counter.css
            css/components/system-status-report-counters.css: css/components/system-status-report-counters.css
            css/components/system-status-report-general-info.css: css/components/system-status-report-general-info.css
            css/components/tabledrag.module.css: css/components/tabledrag.css
    

    In the above code, all of the source system-status-...css will be missing if this MR were to be merged.

    What should we do about these kinds of cases?

  • Status changed to Needs work about 2 years ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    MR has failures.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    From @nod_

    Seems like we could move the details css file to the drupal.collapse library too no? it's a pretty big file in claro.
    Same for sticky-header.module.css that could go into drupal.tableheader maybe?

    Don't think these are already included here, so adding to make sure they get included.

    Also bumping priority here because this is a significant front end performance improvement for any sites that hasn't manually excluded these files itself to get the same effect.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    catch โ†’ changed the visibility of the branch 2880237-refactor-systembase-library to hidden.

  • Merge request !7380Split up system/base library. โ†’ (Open) created by catch
  • Pipeline finished with Failed
    12 months ago
    #141485
  • Pipeline finished with Canceled
    12 months ago
    Total: 43s
    #141519
  • Pipeline finished with Failed
    12 months ago
    #141520
  • Pipeline finished with Failed
    12 months ago
    Total: 763s
    #141522
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Now ๐Ÿ“Œ Move system/base component CSS to respective libraries where they exist Fixed is in, trying to revive this one.

    I've started a new MR because the approach is different. The original approach here used attach from templates, however template overrides would not get the library added so it seemed risky from a bc standpoint. Instead I've moved files between libraries where possible, or if there wasn't an appropriate library (system status et al) into a new library then using #attached in the render element.

    Covered in the MR:

    A new system status report library attached from the status report render element.

    details.module.css moves to core/drupal.collapse

    sticky-header.css moves to core/drupal.tableheader

    A new core/drupal.item_list library attached from the item_list element.

    A new core/drupal.resize library attached from the textarea element.

    Starterkit's tabledrag styling hadn't moved in the previous MR, moved that here.

    Extra things I found:

    Claro's views styling should probably be a libraries-extend of one of the views UI libraries.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    We can re-use the system/base change record we already have as long as this lands in 10.3.x. Tagging for that, and I'll add the suggested text additions to the issue summary once tests are green.

  • Pipeline finished with Failed
    12 months ago
    Total: 669s
    #141563
  • Pipeline finished with Failed
    12 months ago
    Total: 953s
    #141584
  • Pipeline finished with Success
    12 months ago
    Total: 1067s
    #141618
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • Status changed to Needs work 12 months ago
  • 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.

  • Pipeline finished with Failed
    12 months ago
    Total: 1077s
    #148569
  • Pipeline finished with Failed
    12 months ago
    Total: 1113s
    #148585
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Rebased.

  • Pipeline finished with Success
    12 months ago
    Total: 962s
    #148598
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    This is a solid 2kb reduction in CSS payload for most pages now (uncompressed).

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    I'm all for this, can a CR be written up?

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    @smustgrave there's an existing CR for the previous issue at https://www.drupal.org/node/3432346 โ†’

    Since this issue is very similar, I was planning to copy the API changes section of this issue over to that CR once this is committed, as long as it lands in 10.3.x

  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡ซ๐Ÿ‡ทFrance nod_ Lille

    few typos in the library names

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia pradhumanjainOSL

    pradhumanjain2311 โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Success
    12 months ago
    Total: 987s
    #149338
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • Pipeline finished with Success
    12 months ago
    Total: 1081s
    #149929
  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡ซ๐Ÿ‡ทFrance nod_ Lille

    With the patch in the current version (using claro):

    1. item list style missing /admin/reports/fields, the new library is never attached anywhere
    2. tablesort indicator icon missing on claro /admin/content?title=&type=All&status=All&langcode=All&order=status&sort=asc this might need to be a follow-up, looks tricky.
    3. no style on status page /admin/reports/status
    4. There are image paths inside the CSS files that needs to be updated since the path of the css file changed.
    5. Textarea resize styles not applied /admin/structure/types
    6. Haven't found a test case for fieldgroup but that won't work either

    The 2kb makes sense, there is a bunch of file that should be loaded that are not :D

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States kentr Durango, CO

    container-inline.module.css could also be moved, and then attached somewhere in the container chain.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Given the amount of bugs found in #40, and the amount of merge conflicts here after a few months, I think we should split this into individual issues by destination library. Will be easier to verify that way.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Switching to a meta/plan.

    First issue needs review: ๐Ÿ“Œ Move status report assets to their own library Active

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Next one: ๐Ÿ“Œ Move resize CSS into its own library Active .

    Since these all need manual testing and conflict with each other, definitely better to do library-by-library.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    ๐Ÿ“Œ Move fieldgroup CSS to its own library Active theoretically won't conflict with ๐Ÿ“Œ Move resize CSS into its own library Active so added that too.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Added ๐Ÿ“Œ Move details.css to the collapse library Active because that also shouldn't conflict hopefully.

    From the original MR here, this leaves:

    Sticky tableheaders - already done in ๐Ÿ“Œ Make drupal.tableheader only use CSS for sticky table headers Needs review

    Tablesort - needs work issue already at ๐Ÿ“Œ Split tablesort.module.css out to its own library and attach it via tablesort Active

    item-list - @todo.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    That's everything split out to its own issue now.

    Of the remaining files:

    clearfix.module.css - class is used by multiple core demplates.
    align.module.css - used various places in core, but we should remove the views duplicates: ๐Ÿ“Œ Remove the views-align-* CSS classes Active
    container-line.module.css - used in lots of places.
    hidden.module.css - used a lot
    js.module.css - used a lot
    nowrap.module.css - can't see where this is used at all, maybe we can deprecate/drop it.
    position-container.module.css - one usage in claro, could maybe be factored out.
    reset-appearance.module.css - one usage in claro, could be factored out.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Getting close to 1kb of CSS once remaining issues are in, this is down from 7kb originally.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ธ๐Ÿ‡ฎSlovenia KlemenDEV

    Getting close to 1kb of CSS once remaining issues are in, this is down from 7kb originally.

    That is amazing. Nice work!

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Opened ๐Ÿ“Œ [meta] Olivero page weight improvements Active for some similar issues in Olivero and added a couple more child issues here.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
Production build 0.71.5 2024