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

fix: revert to simple icons decoration and extract the spriting logic #253

Closed
wants to merge 7 commits into from

Conversation

ramboz
Copy link
Collaborator

@ramboz ramboz commented Sep 5, 2023

Most customers directly use svg icons as they were explored 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:

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 5, 2023

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 5, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 5, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 5, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@ramboz ramboz marked this pull request as ready for review September 5, 2023 22:02
@aem-code-sync
Copy link

aem-code-sync bot commented Sep 7, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@kptdobe
Copy link
Contributor

kptdobe commented Sep 7, 2023

We are currently exploding lib-franklin into its own library which includes adding tests... I think this refactor would be a perfect fit to add the corresponding tests to the method - See https://github.com/adobe/aem-lib/blob/main/src/decorate.js#L51. Maybe the PR should be moved there.

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 15, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 15, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@kptdobe
Copy link
Contributor

kptdobe commented Sep 15, 2023

@ramboz [https://github.com/adobe/aem-lib is ready (only need to adjust the documentation before release). Could you move your current work in this repo ? We will be able to write corresponding tests to all the icons handling edge cases ;)

@ramboz ramboz changed the title fix: adjust icon spriting logic fix: revert to simple icons decoration and extract the spriting logic Sep 15, 2023
@ramboz
Copy link
Collaborator Author

ramboz commented Sep 18, 2023

@kptdobe Closing in favor of adobe/aem-lib#15

@ramboz ramboz closed this Sep 18, 2023
@davidnuescheler
Copy link
Contributor

i think it would be great to also merge the PR here... i am when we are going to switch over to aem-lib codebase and i just wanted to update the documentation ahead of time...

@kptdobe
Copy link
Contributor

kptdobe commented Oct 2, 2023

aem-lib is now in, the code is now in this repo.

@kptdobe kptdobe closed this Oct 2, 2023
@kptdobe kptdobe deleted the improve-icons branch October 2, 2023 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants