Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

added cdnHost #110

Merged
merged 3 commits into from
Jul 13, 2020
Merged

added cdnHost #110

merged 3 commits into from
Jul 13, 2020

Conversation

dk1027
Copy link
Contributor

@dk1027 dk1027 commented Jul 9, 2020

Added configurable CDN host

This PR added a new prop cdnHost to ConsentManager and ConsentManagerBuilder with a default value of cdn.segment.com which is the same as the existing behavior. This allows users to use their own CDN instead of Segment's.

Testing

Testing is done via unit tests and story book on my local dev environment.

Default behavior is unchanged
image

React component: Setting cdnHost to another CDN
image

Standalone script: Setting cdnHost to another CDN
image

@@ -12,7 +12,7 @@ At its core, the Consent Manager empowers your visitors to control and customize

It works by taking control of the analytics.js load process to only load destinations that the user has consented to and not loading analytics.js at all if the user has opted out of everything. The user's tracking preferences are saved to a cookie and sent as an identify trait (if they haven't opted out of everything) so that you can also access them on the server-side and from destinations (warehouse).

*Segment works to ensure the Consent Manager Tech Demo works with most of our product pipeline. We cannot ensure it works in your specific implementation or website. Please contact our Professional Services team for implementation support. Please see the License.txt included.*
_Segment works to ensure the Consent Manager Tech Demo works with most of our product pipeline. We cannot ensure it works in your specific implementation or website. Please contact our Professional Services team for implementation support. Please see the License.txt included._
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are prettier changes; My changes are adding documentation for cdnHost

@@ -88,7 +88,7 @@
"lodash-es": "^4.17.10",
"nock": "^9.2.5",
"preact": "^10.0.0",
"prettier": "1.18.2",
"prettier": "^1.19.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumping this to the latest 1.x.x version because 1.18.2 fails on the nullish coalescing operator ?? operator

defaultDestinationBehavior={defaultDestinationBehavior}
workspaceAddedNewDestinations={workspaceAddedNewDestinations}
/>
return (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are prettier changes, aside from adding cdnHost.

@dk1027 dk1027 marked this pull request as ready for review July 9, 2020 20:02
@dk1027 dk1027 requested review from notfelineit and pooyaj July 9, 2020 20:02
@dk1027 dk1027 changed the title WIP: added cdnHost added cdnHost Jul 9, 2020
Copy link
Contributor

@pooyaj pooyaj left a comment

Choose a reason for hiding this comment

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

💯

@@ -324,6 +324,49 @@ describe('ConsentManagerBuilder', () => {
)
})

test('a different cdn is used when cdnHost is set', done => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@notfelineit notfelineit left a comment

Choose a reason for hiding this comment

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

Looks good! If possible, it would be great to add an example to the storybook.

@dk1027
Copy link
Contributor Author

dk1027 commented Jul 13, 2020

Hey, since the example will be identical as the two basic examples except for using the new property, I think it would be best if we don't create another example. I've updated the readme so people can discover the cdnHost property.

@dk1027 dk1027 merged commit f23dff7 into master Jul 13, 2020
@dk1027 dk1027 deleted the add-cdn-host branch July 13, 2020 19:20
@pooyaj pooyaj mentioned this pull request Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants