Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(3PT): Add inizio #40

Merged
merged 13 commits into from
Oct 16, 2020
Merged

feat(3PT): Add inizio #40

merged 13 commits into from
Oct 16, 2020

Conversation

mxdvl
Copy link
Contributor

@mxdvl mxdvl commented Oct 7, 2020 •
edited
Loading

What does this change?

Add Inizio third-party tag & adds types on the window.

Why?

Move it out of frontend.

@mxdvl mxdvl requested a review from sndrs October 7, 2020 13:12
@mxdvl mxdvl requested a review from a team as a code owner October 7, 2020 13:12
@mxdvl
Copy link
Contributor Author

mxdvl commented Oct 7, 2020 •
edited
Loading

@sndrs what’s the take on merge commits such as 72c9993—are they chore?

ioanna0
ioanna0 reviewed Oct 8, 2020
View reviewed changes
src/types.ts Outdated Show resolved Hide resolved
ioanna0
ioanna0 reviewed Oct 8, 2020
View reviewed changes
src/types.ts
interface Window {
_brandmetrics?: { cmd: string; val: Record<string, unknown> }[];
// eslint-disable-next-line no-undef
googletag: googletag.Googletag;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice that you found the exact Googletag type 👍 .

@ioanna0
Copy link
Contributor

ioanna0 commented Oct 8, 2020

@sndrs @mxdvl
what happens when 1 PR has multiple different types of commits for semantic releases? I can see 5 different types of commits in this PR, curious how will that work?

@sndrs
Copy link
Member

sndrs commented Oct 8, 2020

what’s the take on merge commits such as 72c9993—are they chore?

yeah chore should be fine i'd think. i've been rebasing locally to avoid this and so far i've not encountered any grim conflict chains

what happens when 1 PR has multiple different types of commits for semantic releases? I can see 5 different types of commits in this PR, curious how will that work?

that's fine - semantic release will examine each commit and decide what it needs to do. you can see it actually examining commits in the build log

sndrs
sndrs reviewed Oct 8, 2020
View reviewed changes
src/third-party-tags/inizio.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
@mxdvl mxdvl requested a review from sndrs October 8, 2020 15:14
@mxdvl mxdvl mentioned this pull request Oct 8, 2020
Closed
@mxdvl
Copy link
Contributor Author

mxdvl commented Oct 9, 2020

I had to manually rebase and git push --force-with-lease to allow Rebase and merge.

@mxdvl mxdvl added the 3PT 🏷️ Third-party tags label Oct 12, 2020
@mxdvl mxdvl changed the title [3pt] Add inizio feat(3pt): Add inizio Oct 12, 2020
@mxdvl mxdvl changed the title feat(3pt): Add inizio feat(3PT): Add inizio Oct 12, 2020
@mxdvl mxdvl self-assigned this Oct 12, 2020
@mxdvl mxdvl requested review from sndrs and ioanna0 October 12, 2020 15:59
ioanna0
ioanna0 approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@ioanna0 ioanna0 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. i think we don't need sourcepointId in the ThirdPartyTag but otherwise can 🚢

@mxdvl mxdvl merged commit c6d35fd into main Oct 16, 2020
@mxdvl mxdvl deleted the 3pt-add-inizio branch October 16, 2020 15:27
@github-actions
Copy link
Contributor

🎉 This PR is included in version 0.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mxdvl
Copy link
Contributor Author

mxdvl commented Jan 14, 2021

This needs some tests added, which caused a coverage failure in #134 when we removed Lotame in #138.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ioanna0 ioanna0 ioanna0 approved these changes

@sndrs sndrs Awaiting requested review from sndrs

Assignees

@mxdvl mxdvl

Labels
3PT 🏷️ Third-party tags released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mxdvl @ioanna0 @sndrs