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

Implement visibility check #48

Closed

Conversation

morgangiraud
Copy link

Pull request associated with this issue: #47

@maxwellito
Copy link
Owner

Thanks for the PR! :)

But I have many points to correct.

  1. Can make the PR to the dev branch please. I'm sorry I should have a CONTRIBUTING.md to define rules.
  2. Well done for updating the tests (usually people don't even think about it), but just updating the SVG to include hidden elements is not a correct test. It deserve it's own it test. I can help about that :)
  3. I thought you would wait the end of the conversation on the Drawing path with stroke attribute set to "none" #47 to purpose a PR. There's few points we need to clarify.

But I want to say thank you because usually people just come and shoot about a feature they want but don't take even a second to implement it, like a customer service. So thanks a lot for contributing :)

@morgangiraud
Copy link
Author

You're right about tests, i was lazy and i liked it that way, after all the path count test of the mapping functions seemed good to me. I can implement a dedicated test when we are ready.

About the PR, i'll wait for more information on the points you want to clarify. I can't change the target branch on Github so i will have to create a new one and close this one. I'll do it when we agree on the spec.

No problem for the PR, seems fair to me to implement what i want ;)

@maxwellito
Copy link
Owner

I'm closing your PR for now. I implemented a basic function to do the job in the last release (v0.2.3).

But this feature is on beta for now, you have to enable it manually to test it. Yes, I'm not really confident about the implementation.
The discussion can continue on the #47

Thanks again for your PR :)

@maxwellito maxwellito closed this Sep 23, 2015
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