Refactor the Popover component #2864
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
sirineJ
changed the title
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
ece7699 to
3863326
Compare
|
Size Change: +106 B (+0.02%) Total Size: 678 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #2864 +/- ##
==========================================
+ Coverage 87.93% 87.96% +0.03%
==========================================
Files 225 226 +1
Lines 12992 13008 +16
Branches 1791 1792 +1
==========================================
+ Hits 11424 11442 +18
+ Misses 1514 1512 -2
Partials 54 54
|
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
3863326 to
20e64a4
Compare
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
20e64a4 to
1353d29
Compare
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
6ce5dd9 to
e1fc6a3
Compare
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
e1fc6a3 to
cc4dd46
Compare
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
cc4dd46 to
38378fd
Compare
sirineJ
commented
Jan 15, 2025
| @@ -131,7 +131,7 @@ describe('Popover', () => { | |||
|
|
|||
| await userEvent.click(popoverTrigger); | |||
|
|
|||
| expect(baseProps.onToggle).toHaveBeenCalledTimes(1); | |||
| expect(baseProps.onToggle).toHaveBeenCalled(); | |||
There was a problem hiding this comment.
This is part of why I was frustrated with onToggle 😄
The Popover closes, but in reality onToggle is called twice: first when the trigger element is clicked (and its value toggled), and then when the onCloseEnd is fired (with false, having no effect).
Having toHaveBeenCalled instead of toHaveBeenCalledTimes(1) is not enough for the test to confirm the popover was closed. I also cannot use toHaveBeenCalledWith(false) because it can get called with a function. I added a TODO to try and improve this later.
There was a problem hiding this comment.
And we have to call onToggle after onCloseEnd because the Dialog might have been closed "natively" (e.g. using the escape key), right?
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
a959b56 to
14213b0
Compare
connor-baer
reviewed
Jan 15, 2025
| @@ -131,7 +131,7 @@ describe('Popover', () => { | |||
|
|
|||
| await userEvent.click(popoverTrigger); | |||
|
|
|||
| expect(baseProps.onToggle).toHaveBeenCalledTimes(1); | |||
| expect(baseProps.onToggle).toHaveBeenCalled(); | |||
There was a problem hiding this comment.
And we have to call onToggle after onCloseEnd because the Dialog might have been closed "natively" (e.g. using the escape key), right?
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
14213b0 to
c3ded5e
Compare
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
c3ded5e to
2e9371d
Compare
connor-baer
approved these changes
Jan 16, 2025
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
2e9371d to
d8ad5c5
Compare
sirineJ
force-pushed
the
feat/DSYS-886/refactor-popover
branch
from
d8ad5c5 to
1e0e086
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #DSYS-886
Purpose
Describe what you are trying to accomplish
Approach and changes
Describe how you solved the problem
Definition of done