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.

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

feat: Add Event Timer module #154

Merged
merged 6 commits into from
Feb 2, 2021
Merged

feat: Add Event Timer module #154

merged 6 commits into from
Feb 2, 2021

Conversation

ashishpuliyel
Copy link
Member

What does this change?

Adds an event timer module that can be imported and used globally.

Why?

As part of the performance OKR we need additional monitoring of performance.
Using this module gives us a single place to collate and handle events recorded for this

@ashishpuliyel ashishpuliyel requested a review from a team as a code owner February 1, 2021 17:01
Copy link
Contributor

@mxdvl mxdvl left a comment

Choose a reason for hiding this comment

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

LGTM.

FYI: you don’t need the ! in the PR title, unless you want to release a patch. feat will do just fine for a minor update.

src/EventTimer.ts Outdated Show resolved Hide resolved
* @param {origin} [origin=page] - Either 'page' (default) or the name of the slot
*/
trigger(eventName: string, origin = 'page'): void {
const TRACKEDSLOTNAME = 'top-above-nav';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be extracted in a higher scope? It could be used in the triggers on lines 23 and 51.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can it be used on line 23?

@ashishpuliyel ashishpuliyel changed the title feat!: Add Event Timer module feat: Add Event Timer module Feb 2, 2021
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.

would be great to have some unit tests on this, but i'm ok to move them on a subsequent PR

@ashishpuliyel
Copy link
Member Author

Adding the event timer module

@ashishpuliyel ashishpuliyel merged commit eee7a61 into main Feb 2, 2021
@ashishpuliyel ashishpuliyel deleted the event-timer branch February 2, 2021 11:40
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2021

🎉 This PR is included in version 0.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ioanna0
Copy link
Contributor

ioanna0 commented Feb 2, 2021

main failed on test coverage step:
COVERAGE DECREASED (-24.4%) TO 17.241%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants