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

Sites with even partial aphrodite usage break loading the polyfill #158

Closed
calinoracation opened this issue Sep 18, 2023 · 3 comments · Fixed by #249
Closed

Sites with even partial aphrodite usage break loading the polyfill #158

calinoracation opened this issue Sep 18, 2023 · 3 comments · Fixed by #249

Comments

@calinoracation
Copy link
Contributor

We're in a case where we're migrating off of Aphrodite to more static CSS solutions, however we still have a significant amount of Aphrodite that hasn't been moved yet. Aphrodite gets really, really broken when you touch it's <style> tag. Khan/aphrodite#376. I don't think there's much chance of it being fixed upstream, so am curious if it would make sense to build in a bail-out on that particular tag in this polyfill.

In the scroll-timeline-css file for handleStyleTag, I added this bail out to make it ignore them:

if (el.innerHTML.trim().length === 0 || el.dataset.aphrodite === 'true') {
  return;
}

Not sure if this specific solution or something else should be considered, but it broke luckily just our staging environment before we found and fixed it. Even doing something ridiculous like el.innerHTML = el.innerHTML breaks Aphrodite.

@flackr
Copy link
Owner

flackr commented Sep 19, 2023

I agree we should have a way to skip the css parsing. The JS parts are also more efficient because they don't have scan anything to detect usage. I was thinking it might be nice to build multiple bundles so that you'd have a smaller script to include if you skip the CSS parsing.

@flackr
Copy link
Owner

flackr commented Sep 19, 2023

if (el.innerHTML.trim().length === 0 || el.dataset.aphrodite === 'true') {
  return;
}

Is this an attribute aphrodite sets? Would it make sense to land this as well?

@calinoracation
Copy link
Contributor Author

@flackr Ahh, yeah that sounds good, just hit this again adopting the new version, so would love to land this.

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 a pull request may close this issue.

2 participants