Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MWPW-140452 - Icon authoring in milo using the federal repo and individual SVG assets #3184

Closed
wants to merge 36 commits into from

Conversation

Sartxi
Copy link
Contributor

@Sartxi Sartxi commented Nov 13, 2024

This is part two of reverted PR: #2986 and makes changes to address issues that were found. I also made some performance updates to the icon decorating functionality.

To streamline icon management across Milo and other consuming sites, a centralized repository, federal, along with a directory named icons/svgs, will be established. This will provide shared access to a unified set of icons. The transition will maintain the current authoring experience, while also introducing new opportunities for contributors to expand the icon set. The key points of this change are outlined below:

Key Points:

Centralized Icon Repository: A new /assets/icons/svgs directory was added to the /federal/ repo, allowing multiple sites (Milo and others) to use the same set of icons. All icons currently available are listed here /icons/icons.json

Consistent Authoring Notation: Authors will continue to use the current notation, such as :icon-play:, to insert icons into content. This ensures a seamless transition with no change in authoring experience.

Source Change: Icons will no longer be served from the Milo code-bus. Instead, they will be fetched from the federal repository via the content-bus.

Author Contributions: This new system enables authors to contribute and expand the icon set by adding new icons to the centralized repository, a feature that was previously unavailable.

Subsequent Ticket: The Sidekick plugin library will be updated in a subsequent ticket to improve authoring accessibility for that component

Resolves: MWPW-140452

Draft URLs:

Doc URLs:

All consuming sites should be tested for this change. ?milolibs=sartxi-fed-icons
https://main--bacom--adobecom.hlx.page/?milolibs=sartxi-fed-icons

ryanmparrish and others added 30 commits September 30, 2024 13:26
…Preloaded inView icons and defered others till postSectionLoad.
@Sartxi Sartxi added new-feature New block or other feature needs-verification PR requires E2E testing by a reviewer labels Nov 13, 2024
Copy link
Contributor

aem-code-sync bot commented Nov 13, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link
Contributor

aem-code-sync bot commented Nov 13, 2024

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.93%. Comparing base (38504d5) to head (1022b16).
Report is 32 commits behind head on stage.

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #3184      +/-   ##
==========================================
- Coverage   96.35%   95.93%   -0.43%     
==========================================
  Files         245      175      -70     
  Lines       56370    48083    -8287     
==========================================
- Hits        54316    46129    -8187     
+ Misses       2054     1954     -100     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 1284 to 1288
const icons = area.querySelectorAll('span.icon');
if (icons.length) {
const { default: decorateIcons } = await import('../features/icons/icons.js');
decorateIcons(icons, config);
}
Copy link
Contributor

@mokimo mokimo Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this is something we wanted to avoid in the initial PR as I had discussed with @saugatmalla and @ryanmparrish as it adds a blocking request rather than having it live in parallel with the other ones. We collected real world data on this and want to avoid sequential blocking requests in front of the LCP chain.

Screenshot 2024-11-13 at 16 58 19

  • We load icons
  • We wait
  • We load federal.js (as import of icons) & the icon after federal finishes, but without blocking anything
  • We wait
  • We can start loading the blocks

What we want is
We load blocks, icons, federal.js, the icons (strictly needed for LCP) all at the same time.

#2986 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay got it, I moved that back to where it was and added an additional query so we aren't loosing the icon elements in tables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-verification PR requires E2E testing by a reviewer new-feature New block or other feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants