Problem/Motivation

After #3366561: Support preloading of fonts in the libraries API, ensure that it also works with components.

Steps to reproduce

Declare a font in a component library overrides, check that it is preloaded.

Proposed resolution

Remaining tasks

- add test coverage for components
- handle new keys properly if needed

User interface changes

None

Introduced terminology

None

API changes

Data model changes

None

Release notes snippet

Issue fork drupal-3586470

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

grimreaper created an issue. See original summary.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

From comment #3565258-70: Support library-specific aggregates

as components libraries are added from core/lib/Drupal/Core/Asset/LibraryDiscoveryParser::librariesForComponents(), I thought updating core/lib/Drupal/Core/Theme/ComponentPluginManager::libraryFromDefinition() to handle those new keys would have done the trick.

grimreaper opened merge request !15669

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Active » Needs review
smustgrave’s picture

smustgrave commented

Should this get a CR?

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

I didn't thought a CR was needed.

Created a draft one https://www.drupal.org/node/3588509

smustgrave’s picture

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

Thanks. For SDC I always lean on CRs as it's always improving trying to keep up is tough!

This appears to be working as advertised so going to mark it.

catch closed merge request !15669

  • catch committed d3c52cf4 on 11.x 
    task: #3586470 Support preloading of fonts declared from components

By...

  • catch committed c0f1c27c on main 
    task: #3586470 Support preloading of fonts declared from components

By...
catch’s picture

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

Good that there's minimal logic for the additional asset type, should mean no extra SDC work when we add early hints and similar support.

Committed/pushed to main and cherry-picked 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.

  • catch committed 7d700639 on main 
    task: #3586470 Support preloading of fonts declared from components

By...
catch’s picture

catch
he/him
Primary language English
commented

@alexpott asked in slack how I determined the font was safe, the answer is I didn't, but he figured out it's Source Sans Pro which is an open font.

However in the process of figuring that out, I realised we don't need a physical font style at all, we just need to check the library definition itself is valid. So went ahead and git rmed it in a follow-up commit.

  • catch committed 14ccb053 on 11.x 
    task: #3586470 Support preloading of fonts declared from components

By...

Status: Fixed » Closed (fixed)

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