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: added twitter pixel & lotame #41

Merged
merged 2 commits into from
Oct 13, 2020
Merged

feat: added twitter pixel & lotame #41

merged 2 commits into from
Oct 13, 2020

Conversation

ioanna0
Copy link
Contributor

@ioanna0 ioanna0 commented Oct 8, 2020

No description provided.

@ioanna0 ioanna0 requested a review from a team as a code owner October 8, 2020 10:47
@ioanna0 ioanna0 changed the title [3PT] chore: added twitter pixel [3PT] chore: added twitter pixel & lotame Oct 8, 2020
@sndrs
Copy link
Member

sndrs commented Oct 8, 2020

because you've used chore for adding twitter and lotame, no new release will be triggered. is this what you expected?

more info here https://www.conventionalcommits.org/en/v1.0.0/#summary

@ioanna0 ioanna0 changed the title [3PT] chore: added twitter pixel & lotame feat: added twitter pixel & lotame Oct 12, 2020
@mxdvl mxdvl added the 3PT 🏷️ Third-party tags label Oct 12, 2020
@sndrs
Copy link
Member

sndrs commented Oct 12, 2020

because you've used chore for adding twitter and lotame, no new release will be triggered. is this what you expected?

no longer true since #53 – ignore this!

@mxdvl
Copy link
Contributor

mxdvl commented Oct 12, 2020 •
edited
Loading

Could we simply ignore the external-scripts folder for ESLint and TypeScript?

@ioanna0
Copy link
Contributor Author

ioanna0 commented Oct 12, 2020

Could we simply ignore the external-scripts folder for ESLint and TypeScript?

Great idea, i've tried that here:
#4d617bf

it is ignoring the Eslint issues but not the TS ones 🤔

mxdvl
mxdvl approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@mxdvl mxdvl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 LGTM

Regarding the exclude directive in .tsconfig, it looks that if a file is imported, it will still be evaluated. Per-file instructions seem like the only way forward.

src/third-party-tags/twitter-uwt.ts Outdated Show resolved Hide resolved
@ioanna0 ioanna0 merged commit 8b6933d into main Oct 13, 2020
@ioanna0 ioanna0 deleted the iky/tpt-migration branch October 13, 2020 09:49
@sndrs
Copy link
Member

sndrs commented Oct 13, 2020

just realised - don't forget to add the export to the index file https://github.com/guardian/commercial-core/blob/main/src/index.ts

@ioanna0
Copy link
Contributor Author

ioanna0 commented Oct 13, 2020

oh yes forgot about it ! fixed here #55

@sndrs
Copy link
Member

sndrs commented Oct 13, 2020

🎉 This PR is included in version 0.7.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sndrs sndrs added the released label Oct 13, 2020
@ioanna0 ioanna0 mentioned this pull request Oct 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sndrs sndrs sndrs left review comments

@mxdvl mxdvl mxdvl approved these changes

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

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