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

rewrite localhost domain for serialized assets to render.percy.local #1278

Merged
merged 11 commits into from
Jun 21, 2023

Conversation

shahidk8
Copy link
Contributor

@shahidk8 shahidk8 commented Jun 16, 2023

Issue: #1246

Difficulty in showing canvas serialized images on safari browser. Fixed by replacing localhost with render.percy.local

read more here https://docs.percy.io/docs/browsers-specific-handling#localhost-proxy-support-on-safari

@shahidk8 shahidk8 requested a review from itsjwala June 16, 2023 14:03
@itsjwala itsjwala added the 🐛 bug Something isn't working label Jun 16, 2023
Copy link
Contributor

@itsjwala itsjwala left a comment

Choose a reason for hiding this comment

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

Nice @shahidk8, back to you

  • update title to rewrite localhost domain for serialized assets to render.percy.local
  • Add more description to the description -> background, link our public docs 'why is it an issue?', what are we fixing e.t.c
  • Add tests and fix lints error.

@shahidk8 shahidk8 changed the title dom transform localhost url rewrite localhost domain for serialized assets to render.percy.local Jun 19, 2023
Copy link
Contributor

@itsjwala itsjwala left a comment

Choose a reason for hiding this comment

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

Back to you @shahidk8

packages/dom/src/utils.js Outdated Show resolved Hide resolved
packages/dom/test/utils.test.js Show resolved Hide resolved
packages/dom/src/utils.js Outdated Show resolved Hide resolved
packages/dom/src/utils.js Outdated Show resolved Hide resolved
@itsjwala itsjwala marked this pull request as ready for review June 20, 2023 07:08
packages/dom/src/utils.js Outdated Show resolved Hide resolved
packages/dom/src/utils.js Outdated Show resolved Hide resolved
packages/dom/src/utils.js Outdated Show resolved Hide resolved
packages/dom/src/utils.js Outdated Show resolved Hide resolved
Copy link
Contributor

@itsjwala itsjwala left a comment

Choose a reason for hiding this comment

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

Looks good 🎉

@itsjwala itsjwala merged commit 76311b0 into master Jun 21, 2023
30 of 32 checks passed
@itsjwala itsjwala deleted the safari-localhost-issue branch June 21, 2023 10:09
shahidk8 added a commit that referenced this pull request Jun 29, 2023
…1278)

* dom transform localhost url

* added test cases and resolved lint issues

* lint issues

* resolved lint issues

* PR comment changes

* lint issues

* changes from review

* review changes

* lint resolved

* review changes

* lint resolve

---------

Co-authored-by: “Shahid <“shahid.k@browserstack.com”>
samarsault pushed a commit that referenced this pull request Jun 29, 2023
…1278)

* dom transform localhost url

* added test cases and resolved lint issues

* lint issues

* resolved lint issues

* PR comment changes

* lint issues

* changes from review

* review changes

* lint resolved

* review changes

* lint resolve

---------

Co-authored-by: “Shahid <“shahid.k@browserstack.com”>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants