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 mapping of CSS URLs #33

Merged
merged 8 commits into from
Nov 13, 2023
Merged

Fix mapping of CSS URLs #33

merged 8 commits into from
Nov 13, 2023

Conversation

tevanoff
Copy link
Contributor

@tevanoff tevanoff commented Nov 9, 2023

Issue: CAP-1329

What Changed

When we need to rewrite an asset path, we're currently only updating the src attribute on DOM nodes. If an asset loaded from a CSS file or style attribute was rewritten, it will fail to load because the CSS will reference the old path when the snapshot is rendered.

This goes through the contents of style attributes and style elements and replaces any url references as needed.

There's still one empty box in the E2E Storybook due to a missing asset, but that's due to CAP-1194. I believe getting rid of the rrweb fork, which can be a follow up to this, will resolve that.

How to test

  • Run canary against some test projects
  • Check out Chromatic build

Change Type

  • maintenance
  • documentation
  • patch
  • minor
  • major
📦 Published PR as canary version: 0.0.37--canary.33.f9c5b5b.0

✨ Test out this PR locally via:

npm install @chromaui/test-archiver@0.0.37--canary.33.f9c5b5b.0
# or 
yarn add @chromaui/test-archiver@0.0.37--canary.33.f9c5b5b.0

@tevanoff tevanoff force-pushed the todd/fix-css-attribute-mapping branch from 07977d7 to f9c5b5b Compare November 9, 2023 23:26
@tevanoff tevanoff marked this pull request as ready for review November 9, 2023 23:27
Copy link
Member

@skitterm skitterm left a comment

Choose a reason for hiding this comment

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

LGTM!

src/write-archive/index.ts Show resolved Hide resolved
Copy link
Member

@thafryer thafryer left a comment

Choose a reason for hiding this comment

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

LGTM. A future improvement here would be ensuring that assets which can be shared across captures/archives are deduped. However, no changes needed yet!

@tevanoff tevanoff merged commit d56b4fc into main Nov 13, 2023
4 checks passed
@thafryer
Copy link
Member

🚀 PR was released in v0.0.37 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants