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

Allow absolute paths for snapshot directories #453

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ktsn
Copy link

@ktsn ktsn commented Jun 1, 2017

Hi, thank you for the great tool! I'm using it in some projects and it is really helpful!

I'm making this PR because I realized BackstopJS does not handle absolute paths in paths.bitmaps_reference and paths.bitmaps_text. This is not a problem in ordinary projects but I'm facing the case that needs to use absolute paths in it. This PR allows us to set absolute paths in the config.

You can confirm the current behavior by the following reproduction.
https://github.com/ktsn/backstop-compare-test

By the way, I'm not sure how we should write test code for such the fix. If it is needed, please let me know.

Thanks!

@garris
Copy link
Owner

garris commented Jun 1, 2017

Thanks for the PR. could you add this... https://github.com/ktsn/backstop-compare-test to the examples directory. Maybe name it absolute-paths.

@ktsn
Copy link
Author

ktsn commented Jun 2, 2017

@garris Added it into the examples directory with some tweaks.

@garris
Copy link
Owner

garris commented Jun 4, 2017

Thank you for posting this. I am planning to promote the current canary to @latest this week. I will merge this into canary after promoting the current so we can test it out. I am just a little concerned this could break existing implementations so I want others to test it for a while. Cheers.

Copy link
Owner

@garris garris 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. Thanks for you patience!

@garris
Copy link
Owner

garris commented Jun 28, 2017

The files have changed significantly so I will merge this by hand. I will close the ticket when complete.

garris added a commit that referenced this pull request Jun 28, 2017
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.

2 participants