-
Notifications
You must be signed in to change notification settings - Fork 68
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
feat(react): Add support for custom logger #181
Conversation
@jeeyyy LGTM but do we want to start adding more tests? If so, a test for this change would be good. |
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.
@guidokessels thanks for the PR ⭐
Could you add some tests please?
@guidokessels any updates on adding tests to these? I will ear mark this PR as WIP for now, please remove WIP from title once you have added the tests and updated the PR. |
@dylanb IMO it's very unreasonable to ask contributors to add tests. The current test suite is extremely lacking and very confusing. Furthermore, it doesn't actually "work" (IIRC you can comment out Just tried to |
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.
Thank you very much @guidokessels for your contribution!
At the moment
@axe-core/react
will always log the results to the console. We have a use case where we want to handle the results in a different way. To be able to do this I added a new optional parameter which can be used to handle the results from@axe-core/react
. If this parameter is not defined it will fall back to logging to console.Reviewer checks
Required fields, to be filled out by PR reviewer(s)