search

adobe / aem-lib

Apache License 2.0
2 stars 7 forks source link

fix: revert icons to simple decoration #15

Closed ramboz closed 1 year ago

ramboz commented 1 year ago

Moving https://github.com/adobe/helix-project-boilerplate/pull/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:

Related Issue

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

Checklist:

kptdobe commented 1 year ago

Nice, thanks @ramboz!

kptdobe commented 1 year ago

I still think we could integrate the test from https://github.com/adobe/aem-lib/pull/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

kptdobe commented 1 year ago

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 https://github.com/adobe/helix-project-boilerplate/pull/192. Maybe adding a test for each of them (or make sure they are covered by a test) before merging would be awesome.

ramboz commented 1 year ago

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 commented 1 year ago

:tada: This PR is included in version 1.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: