-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Just some points to add in the Readme to make it clear how to use the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm tbh I don't see much value in this being a package, rather than just living at the root level of the repo - it's not a component, and it's not a utility that will be consumed by components. Not something we'll need versioning for.
e.g. will we be using this code directly to run tests, or will we have an npm install @bbc/psammead-test-helpers
in our CI? Is that confusing?
I don't feel too strongly about it, but want to know that we've considered it first. 🤔
@ChrisBAshton We'll need these test helpers in both Psammead and in Simorgh, since we'll still have Containers in Psammead that need snapshot tests. For that reason, it makes sense to have this as a single package that can be used by both repositories. In future, other teams with their own component libraries could use the package as a quick way to get started with their tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair 👍
Test helpers are required for #79 so having them as a package is useful IMO
[ ] Tests added for new features[ ] Test engineer approval