feat: Add a useTagEditor hook to control the diffing of tags #2733
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2733 +/- ##
==========================================
+ Coverage 95.90% 95.91% +0.01%
==========================================
Files 748 750 +2
Lines 20732 20915 +183
Branches 7059 7123 +64
==========================================
+ Hits 19882 20061 +179
- Misses 794 798 +4
Partials 56 56 ☔ View full report in Codecov by Sentry. |
|
|
||
| useEffect(() => { | ||
| if (initialTags.length === 0) { | ||
| console.warn('Warning: initialTags is empty.'); |
There was a problem hiding this comment.
We have an internal util (warnOnce) for this, to keep warnings from repeating every render or messing up console output in production.
| useEffect(() => { | ||
| if (initialTags !== initialTagsRef.current) { | ||
| console.warn('Warning: initialTags has changed in the useTagEditor hook. The internal state will be reset.'); | ||
| initialTagsRef.current = initialTags; |
There was a problem hiding this comment.
This feels dangerous. A state variable just for a dev warning is good enough. I don't think we need to reset the internal state if this happens.
| initialTags.some(({ key, value }) => tag.key === key && tag.existing && tag.value !== value) | ||
| ); | ||
|
|
||
| const onChange = useCallback((event: NonCancelableCustomEvent<TagEditorProps.ChangeDetail>) => { |
There was a problem hiding this comment.
Do we need this function to be stable? Otherwise, useCallback here is probably just useless.
Description
Related links, issue #, if available: n/a
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
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.