fix(cdk:popper): reference hidden popper are no longer updated #1515
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| return | ||
| } | ||
|
|
||
| updatePopperPosition(state) | ||
| } | ||
|
|
||
| let cleanUpHandler: (() => void) | null = null |
There was a problem hiding this comment.
with a brief explanation of the code patch
The code patch is a piece of code from an application that provides an API for users to use. The code patch is responsible for updating the position of a "popper" element (a small component that appears when the user interacts with the application) based on the options provided by the user and the condition of the visibility element. It uses the computePosition function to calculate the position and then updates the position accordingly. Furthermore, it checks if the reference is hidden or not, and if so, it will not update the position of the popper element.
|
This preview will be available after the AzureCI is passed. |
| ...res.data, | ||
| referenceHidden, | ||
| }, | ||
| } | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
code review.
First of all, the code looks well structured and organized. The comments on the code also helps to understand the code better.
The code is using the hide() method from the @floating-ui/dom library and adding a new middleware called referenceHidden to add an attribute to the "floating" element when it is hidden.
There are no obvious bugs in the code, however there are some improvements that can be made.
- The sides array can be moved outside of the referenceHidden() function to make it more accessible and make the code easier to maintain.
- The referenceHiddenOffsets object can be validated to make sure that only valid sides are used.
- The referenceHiddenOffsets object should also be validated to make sure that the values are within the expected range.
- The code should also be tested to make sure that the expected behavior is working correctly.
| @@ -76,7 +76,7 @@ export function usePopper<TE extends PopperElement = PopperElement, PE extends P | |||
|
|
|||
| function initialize() { | |||
| destroy() | |||
| popperInstance = useInstance(triggerRef, popperRef, convertedOptions) | |||
| popperInstance = useInstance(triggerRef, popperRef, visibility, convertedOptions) | |||
| } | |||
|
|
|||
| function destroy(): void { | |||
There was a problem hiding this comment.
by looking at the diff between the two code snippets.
The change here is that a new parameter called "visibility" is being passed to useInstance. This could potentially cause bugs if this parameter isn't handled properly, or if the implementation of useInstance doesn't accept it. Additionally, it's not clear what purpose this parameter is intended to serve, so it's worth investigating why it was added to the code.
It's also important to ensure that the calling code (which is presumably outside of the snippet we're looking at) is passing the correct value for the visibility parameter. This could be done with an assertion or a check to make sure the value is valid.
Finally, it's worth considering whether the new parameter is necessary at all. If it's not being used in any way, then it may be better to remove it altogether as it could cause confusion and clutter.
Codecov Report
@@ Coverage Diff @@
## main #1515 +/- ##
==========================================
+ Coverage 92.75% 92.78% +0.03%
==========================================
Files 331 332 +1
Lines 30801 30879 +78
Branches 3533 3555 +22
==========================================
+ Hits 28568 28651 +83
+ Misses 2233 2228 -5
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
PR Checklist
Please check if your PR fulfills the following requirements:
What is the current behavior?
reference hidden 的 popper，位置依然会被计算，此时会把浮层的位置计算到左上角
What is the new behavior?
Other information