feat: use aem.js #255
feat: use aem.js #255
Conversation
|
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
|
|
|
|
|
Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
|
|
|
| const iconName = Array.from(span.classList) | ||
| .find((c) => c.startsWith('icon-')) | ||
| .substring(5); | ||
| const img = document.createElement('img'); |
There was a problem hiding this comment.
By using the img tag to load the SVG, you lose the ability to apply CSS to change its color and other properties. This is a common use case for small icons that for example should match the surrounding text. For example on https://blog.adobe.com/en/publish/2023/10/11/max-sneaks-2023, checkout the share icons.
In some cases, using the img tag to load larger SVG illustrations makes sense as you don't want to increase the DOM size unnecessarily.
So depending on the use case, ideally you should be able to decide whether you want to load the SVG with <svg> or with <img> WDYT ?
There was a problem hiding this comment.
Also this change should have been done in a separate PR IMO. It's hidden behind the franklin to aem rename currently.
There was a problem hiding this comment.
@icaraps We've had many issues and edge cases with the SVG sprite approach we had introduced earlier and the decision was taken to go back to a simple decoration that non-experts can leverage out-of-the-box for GA.
(check adobe/aem-lib#15 for more details)
Since styling icons essentially requires that the SVG file be modified from the OOTB export that apps like Illustrator give you, it is assumed to be a developer use case that can be done directly at the project level for now. And we are planning to introduce a plugin system in the near future that would allow you to pull in the sprite logic back on-demand.
There was a problem hiding this comment.
@ramboz Ok thanks for the details. Do you have recommendations on how to best migrate projects using svg icons to images instead ? For example, the icon selector Sidekick plugin built for Nokia uses svg icons https://main--nokia--hlxsites.hlx.page/tools/sidekick/library.html?plugin=icons. Do you foresee any issues if they switch to use images instead ?
There was a problem hiding this comment.
First, I'd try to use the new logic and see if it actually breaks anything. As far as I can see from the code, you weren't using the SVG sprite approach that we removed, and the change for you is just inline svg to SVG wrapped in an <img/> tag. It's likely the only impact would be the currentcolor inheritance from the spectrum themes, which might be easily searched/replaced globally if you don't use multiple variations of it.
If you do see regressions with the change, I'd suggest just copying over your current decorateIcons logic into the project code and leverage that instead of the central default one.
# [1.3.0](v1.2.2...v1.3.0) (2023-11-30) ### Bug Fixes * attempt to fix release ([5bb8f73](5bb8f73)) * **build:** fix org in issues URL ([7226d84](7226d84)) * **build:** no double slashes in issues url ([1191064](1191064)) * **github:** increase default permissions for cleanup action ([f8ed051](f8ed051)) * **github:** restrict running of action to initial commit on main branch ([1a990c2](1a990c2)) * icons no more svgs ([#267](#267)) ([2d4cfa7](2d4cfa7)) * **lib:** update scripts/aem.js to aem.js@1.3.2 ([#266](#266)) ([2d61c6a](2d61c6a)) * rescope icon identifiers to avoid clashing references across icons ([#241](#241)) ([e8dc533](e8dc533)), closes [#235](#235) * sampleRUM always for checkpoint to be called even if RUM not selected ([fcca39d](fcca39d)) * trigger release ([afbd201](afbd201)) ### Features * add capability to patch block config at runtime ([#246](#246)) ([a774acc](a774acc)) * enable listeners to all RUM events, not just the sampled ones ([871ede4](871ede4)) * use aem.js ([#255](#255)) ([f5ab4df](f5ab4df))
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Use aem.js built from https://github.com/adobe/aem-lib.
Requires a rename of the repo from
helix-project-boilerplatetoaem-boilerplateright after merging.Test URLs: