-
Notifications
You must be signed in to change notification settings - Fork 8
SDKNEW-2635 - Added support for AD #268
Conversation
ori-gold-px
left a comment
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.
Overall the code looks good! I also like that you used a linter :)
Two notes:
(1) Part of the user_identifiers feature is to handle the pxcts cookie but I didn't see that included in this PR. If it's unintentional, could you add support for that as well? Description here: https://github.com/PerimeterX/px-enforcer-spec/wiki/User-Identifiers
(2) In feature PRs we also add to the README / CHANGELOG / px_metadata if they exist so that when we release a version we don't need to go back to the previous version and compare all the added features; everything will be documented and updated already. I think only CHANGELOG is relevant for this repo -- could you add it? Thanks!
ori-gold-px
left a comment
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! One last little change and then it's good to go!
No description provided.