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

Adds support for interaction components #16

Closed
wants to merge 2 commits into from

Conversation

garemoko
Copy link

I didn't create this and haven't tested it.

I'm making this PR on request of Oliver Gurnell, Technical Director at Page Tiger. I assume @paul-carpenter is a developer at Page Tiger.

Page Tiger have written this code so that they can include interaction component properties in their statements.

*
* Includes support for Activity Interactions
*
*/
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer to keep this out of the source, git should be able to track these types of changes easily. If we need new attribution it can go somewhere outside of the source itself.

Copy link
Author

Choose a reason for hiding this comment

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

I'll check with Oliver about this. It may be that this was just intended to be an internal note so they know what they changed from the library, which becomes unnecessary if/when this is merged anyway.

@brianjmiller
Copy link
Member

Current test coverage isn't great, but I'd like to see at least minimal effort put into including unit tests, particularly for whole new classes where added. At some point we have to stop accepting submissions that aren't even making an effort towards test coverage (including my own), or we'll never have confidence in what is in there. Not sure it is a deal breaker, but it may sit for a while without them, particularly in light of just having tagged a release.

@garemoko
Copy link
Author

Having discussed this with Oliver, it transpires that this code I found looking at forks is in fact not the code from Page Tiger. It turns out that Oliver hadn't in fact published his code to Github yet. This in fact is just by coincidence code covering the exact same need and following a similar approach - close enough for me to fail to spot the differences between this and the code files Oliver mailed me. Oliver is going to take @brianjmiller's comments on board as many also relate to his own code and then have another try at publishing to GitHub and making a PR.

Sorry for my confusion.

@brianjmiller
Copy link
Member

I'm closing this in favor of #17 which is Oliver's work, if that shouldn't be the case please leave a comment.

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.

3 participants