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.

Sign up for GitHub

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

Jump to bottom

fix: svg polygons not showing up; using DOMParser for svg #192

Closed
wants to merge 8 commits into from
Closed

fix: svg polygons not showing up; using DOMParser for svg #192

wants to merge 8 commits into from

Conversation

dicagno
Copy link

@dicagno dicagno commented Mar 16, 2023 •
edited
Loading

fix: svg polygons not showing up; using DOMParser instead of regex for svg manipulation, for improved safety

Fix #191

Test URL: https://fix-decorated-svg--helix-project-boilerplate--dicagno.hlx.page/

@dicagno
fix: svg polygons not showing up in some decorated svg elements; usin…
a104651 
…g DOMParser instead of regex for improved safety
@aem-code-sync
Copy link

aem-code-sync bot commented Mar 16, 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 Mar 16, 2023

Page Score PSI Audit Google
/ Lighthouse returned error: ERRORED_DOCUMENT_REQUEST. Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: 404) PSI

herzog31
herzog31 approved these changes Mar 16, 2023
View reviewed changes
scripts/lib-franklin.js Outdated Show resolved Hide resolved
ramboz
ramboz reviewed Mar 16, 2023
View reviewed changes
Copy link
Collaborator

@ramboz ramboz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would also try to validate this against an icon-heavy page to be sure the perf impact isn't perceptible.

https://github.com/BambooHR/bamboohr-website/pull/75 was the starting point for this initial work, so you could check against their page to measure the impact.

scripts/lib-franklin.js Outdated Show resolved Hide resolved
@aem-code-sync
Copy link

aem-code-sync bot commented Mar 17, 2023

Page Score PSI Audit Google
/ Lighthouse returned error: ERRORED_DOCUMENT_REQUEST. Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: 404) PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Mar 17, 2023

Page Score PSI Audit Google
/ Lighthouse returned error: ERRORED_DOCUMENT_REQUEST. Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: 404) PSI

@nc-andreashaller nc-andreashaller mentioned this pull request Mar 17, 2023
Open
@aem-code-sync
Copy link

aem-code-sync bot commented Mar 17, 2023

Page Score PSI Audit Google
/ Lighthouse returned error: ERRORED_DOCUMENT_REQUEST. Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: 404) PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Mar 17, 2023

Page Score PSI Audit Google
/ Lighthouse returned error: ERRORED_DOCUMENT_REQUEST. Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: 404) PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Mar 17, 2023

Page Score PSI Audit Google
/ Lighthouse returned error: Something went wrong. PSI

@dicagno
Copy link
Author

dicagno commented Mar 17, 2023 •
edited
Loading

Tested the latest changes on a couple of different use cases, some svg paths are not displayed. Working on fixing this.

@aem-code-sync
Copy link

aem-code-sync bot commented Mar 17, 2023

Page Score PSI Audit Google
/ Lighthouse returned error: ERRORED_DOCUMENT_REQUEST. Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: 404) PSI

@dicagno dicagno requested a review from herzog31 March 17, 2023 16:29
@dicagno dicagno requested a review from ramboz March 20, 2023 09:50
@dicagno dicagno changed the title fix: svg polygons not showing up in some decorated svg elements; usin… fix: svg polygons not showing up; using DOMParser for svg Mar 20, 2023
@dicagno dicagno closed this Mar 21, 2023
@dicagno dicagno deleted the fix/decorated-svg branch March 21, 2023 14:27
@dicagno dicagno restored the fix/decorated-svg branch March 21, 2023 14:28
@dicagno dicagno reopened this Mar 21, 2023
@aem-code-sync
Copy link

aem-code-sync bot commented Mar 28, 2023

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

ramboz added a commit that referenced this pull request Sep 7, 2023
@ramboz
chore: address PR feedback and incorporate #192
4bffdbd
@ramboz ramboz mentioned this pull request Sep 18, 2023
Merged
10 tasks
@dicagno dicagno closed this by deleting the head repository Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@herzog31 herzog31 herzog31 approved these changes

@ramboz ramboz Awaiting requested review from ramboz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

decorateIcons() accidentally hides polygons from SVG elements
3 participants
@dicagno @ramboz @herzog31