-
Notifications
You must be signed in to change notification settings - Fork 155
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
chore: Emit component updated metrics during table interactions #2901
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2901 +/- ##
==========================================
- Coverage 96.23% 96.23% -0.01%
==========================================
Files 767 767
Lines 21699 21703 +4
Branches 7363 7428 +65
==========================================
+ Hits 20882 20885 +3
- Misses 764 765 +1
Partials 53 53 ☔ View full report in Codecov by Sentry. |
import { useEffectOnUpdate } from '../use-effect-on-update'; | ||
import { useRandomId } from '../use-unique-id'; | ||
|
||
function useTaskInteractionAttribute(elementRef: React.RefObject<HTMLElement>, value: string) { |
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.
We already have this code in another place:
components/src/internal/hooks/use-performance-marks/index.ts
Lines 13 to 27 in 56aa09e
function usePerformanceMarkAttribute(elementRef: React.RefObject<HTMLElement>, value: string) { | |
const attributeName = 'data-analytics-performance-mark'; | |
const attributeValueRef = useRef<string | undefined>(); | |
useEffect(() => { | |
// With this effect, we apply the attribute only on the client, to avoid hydration errors. | |
attributeValueRef.current = value; | |
elementRef.current?.setAttribute(attributeName, value); | |
}, [value, elementRef]); | |
return { | |
[attributeName]: attributeValueRef.current, | |
}; | |
} |
Can we merge these two into one, to reduce the risk of diverging implementations (e.g. when we make bug fixes)?
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.
Fair point. Mine was based off of this one. Will consolidate into a useDOMAttribute
hook.
@@ -182,12 +182,25 @@ export interface IPerformanceMetrics { | |||
taskCompletionData: TaskCompletionDataMethod; | |||
} | |||
|
|||
type JSONValue = string | number | boolean | null | undefined; |
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.
When the value of an object's property is undefined
, the property will not appear in the JSONified string at all - is that okay for our metrics? Should we enforce values being null
instead?
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.
I had mostly tried to align with this type interface. I think not appearing in the string is fine.
metadata.current = { itemCount, getComponentIdentifier, getComponentConfiguration, interactionMetadata }; | ||
|
||
useEffect(() => { | ||
ComponentMetrics.componentMounted({ |
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.
With every taskInteractionId
change, do we wanna track mount?
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.
const taskInteractionId = useRandomId();
useRandomId()
returns a ref value so this does not change on render and the effect will only run once on mount. In theory I could leave the dependency out but then I would need to have an eslint-disable comment.
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.
I think as long as it's a ref it should be fine, but I guess that also relies on the assumption that useRandomId
is always going to return a ref.
...nternal/hooks/use-table-interaction-metrics/__tests__/use-table-interaction-metrics.test.tsx
Outdated
Show resolved
Hide resolved
...nternal/hooks/use-table-interaction-metrics/__tests__/use-table-interaction-metrics.test.tsx
Outdated
Show resolved
Hide resolved
4895306
to
30c0f00
Compare
30c0f00
to
9fcb634
Compare
Description
How has this been tested?
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.