fix: revert icons to simple decoration

[ramboz]

Moving adobe/aem-boilerplate#253 over to the new repo.

Description

Most customers directly use svg icons as they were exported by their preferred application (Illustrator, etc.), and that markup isn't compatible with the spriting logic in most cases. On top of that, the current spriting implementation has required frequent patching and isn't likely that stable yet. As such, we want to revert to basic icons decoration by default, and move the spriting to the block party instead until it is considered mature enough.

Test URLs:

• Before: https://main--aem-lib--adobe.hlx.page/
• After: https://simplify-icons--aem-lib--adobe.hlx.page/

Related Issue

• fix: svg polygons not showing up; using DOMParser for svg aem-boilerplate#192
• Update lib-franklin.js to fix issue with snake case icons aem-boilerplate#213
• Icon no dupes aem-boilerplate#220
• fix: rescope icon identifiers to avoid clashing references across icons aem-boilerplate#241
• fix: race condition in decorateIcons aem-boilerplate#256

Motivation and Context

For the spriting to really work, it requires:

1. that icons leverage currentcolor
2. that icons not be styled (no <style> tag or class attributes)
3. that all identifiers be properly scoped to avoid clashes between different icons
4. that icon sizes be specifically applied via CSS

All of which require developer involvement and do not apply to the basic use cases. Hence we want to simplify the reference implementation and rather move the spriting approach to the block party.

How Has This Been Tested?

This is how the behavior used to be before we introduced the spriting logic. Additionally, this has been tested against the boilerplate, and also with BambooHR's icon-heavy pages which initially triggered the spriting discussion

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• I have signed the Adobe Open Source CLA.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[kptdobe]

Nice, thanks @ramboz!

[kptdobe]

I still think we could integrate the test from #12 - the method is not async anymore which somehow invalidates the test, I agree but for backward compatibility of the code, I still think it could be good to just add the test.
@ramboz WDYT ?

[ramboz]

Sure, it will make it both back/forward compatible in a sense and guarantee the behavior even if we iterate over the implementation. We do have to remove the sprite-related checks from the test though

[kptdobe]

Yes, of course, the test has to be adapted to the new implementation. I was more referring to the "concurrent" aspect, just to be sure it does not break if you load the same icon twice in parallel!

[ramboz]

@kptdobe Are you guys handling the merge here, or should I just go ahead?

[kptdobe]

I would prefer to integrate the tests from #12 in this PR so that we have a better coverage right for the beginning.
I have also seen you reference a bunch of issue in the description like adobe/aem-boilerplate#192. Maybe adding a test for each of them (or make sure they are covered by a test) before merging would be awesome.

[kptdobe]

Changing my PR review to increase the test coverage based on the list of issues mentioned in the PR description.

[ramboz]

I'm not sure any of those would really apply anymore. They were specific edge cases with the SVG sprite implementation. So more white-box tests than black-box ones. and the last one is already covered by #12

[kptdobe]

ok, let's merge it and continue from here!

[kptdobe]

🎉 This PR is included in version 1.2.1 🎉

The release is available on:

• GitHub release
• v1.2.1

Your semantic-release bot 📦🚀