-
Notifications
You must be signed in to change notification settings - Fork 164
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 analytics metadata to Flashbar #2687
Conversation
8e26524
to
8ef8dd8
Compare
8ef8dd8
to
d4a7dcf
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2687 +/- ##
==========================================
- Coverage 95.85% 95.84% -0.01%
==========================================
Files 744 745 +1
Lines 20581 20596 +15
Branches 7015 6649 -366
==========================================
+ Hits 19727 19741 +14
- Misses 846 847 +1
Partials 8 8 ☔ View full report in Codecov by Sentry. |
className={styles['dismiss-button-wrapper']} | ||
{...getAnalyticsMetadataAttribute({ | ||
action: 'dismiss', | ||
} as Partial<GeneratedAnalyticsMetadataFlashbarDismiss>)} |
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.
Two questions about this line:
- We defined
GeneratedAnalyticsMetadataFlashbarDismiss
interface but its only use case is in this line withPartial<...>
. Why we don't just mark thelabel
as optional? - Why we don't use a creator functions like the other cases? something like
getDismissButtonAnalyticsMetadata
which returns this type like others?
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.
Thank you for the comments:
- The output the function that collects the metadata will have a label. We use this attribute here to override the
action
part of it, which would be a genericclick
. I prefer to keep the type aligned with the outcome of the processing, so that I can automatically generate documentation in the future. - More than 15 components are already instrumented, and I normally didn't need to create specific functions to collect the metadata. The Flashbar was an exception because of the double implementation (collapsible and non-collapsible. However, if you prefer I can create a function. Let me know.
Description
Similar to 9c17527
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.