-
Notifications
You must be signed in to change notification settings - Fork 45
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.
Small things.
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.
1 small thing. This LGTM tho!
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.
LGTM.
Please get a second approval on this tho. I am not very familiar with this project.
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.
Noticed 2 minor things
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.
I think this looks good, I don't see anything glaring
Has this been released? Version pulled from npm does not include type declarations and is listed on npmjs.com as 3.4.1, whereas develop is currently at 3.4.0? |
It does not appear types was released to npm. As for the version discrepancy, seems we never pulled master back into develop after the release. |
This one was a bit tricky with all the private property lookups in axe-core and React.
Reviewer checks
Required fields, to be filled out by PR reviewer(s)