-
Notifications
You must be signed in to change notification settings - Fork 138
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
fix(comp:select): compositionEnd can't trigger #1513
Conversation
@@ -47,6 +53,7 @@ export default defineComponent({ | |||
value={inputValue.value} | |||
onCompositionstart={handleCompositionStart} | |||
onCompositionend={handleCompositionEnd} | |||
onKeydown={handleKeyDown} | |||
onInput={handleInput} | |||
/> | |||
{multiple && <span ref={mirrorRef} class={`${prefixCls}-mirror`} aria-hidden></span>} |
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.
code review:
- The code looks correct and is properly indented.
- The handleKeyDown function is correctly defined and uses the correct event type for the KeyboardEvent.
- The onKeyDown event handler is correctly attached to the input element.
- The event handler is correctly checking the event code property to detect if the enter key has been pressed.
- The handleCompositionEnd function is correctly called when the enter key is pressed.
- The readonly computed value looks correct and is properly used to control the input readonly attribute.
- The mirrorRef ref is correctly used to create a hidden span element.
Overall, the code looks good and there are no major issues that could cause bugs. However, some minor improvements can be made. For example, it would be better to use the key property instead of the code property when checking for the enter key. Additionally, it would be good to add type annotations for the handleKeyDown function parameters.
This preview will be available after the AzureCI is passed. |
@@ -137,6 +138,7 @@ export default defineComponent({ | |||
handleCompositionStart, | |||
handleCompositionEnd, | |||
handleInput, | |||
handleEnterDown, | |||
}) | |||
|
|||
const elementRef = ref<HTMLDivElement>() |
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 the code review
-
The useInputState function should be checked to make sure that it is properly implemented and not introducing any bugs, such as incorrect data being passed in.
-
The handleEnterDown function should be tested to make sure that it correctly handles user input, such as when the user presses the enter key.
-
The getBoundingClientRect() function should be tested to make sure that it is returning the correct values.
-
The use of the ref() should be verified to make sure that it is not creating any memory leaks.
-
The overall structure of the code should be reviewed to make sure that it is following best practices and is well organized.
@@ -116,5 +124,6 @@ export function useInputState(props: SelectorProps, mergedSearchable: ComputedRe | |||
handleCompositionEnd, | |||
handleInput, | |||
clearInput, | |||
handleEnterDown, | |||
} | |||
} |
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.
the code review.
- From my point of view, the code looks clean and easy to understand.
- The handleEnterDown function is a good addition to the InputStateContext interface as it handles the issue of the enter key not triggering the compositionEnd event when using a Chinese input method.
- The handleInput function has an extra parameter for emitting the input event. This could be removed since the emitInput variable is always set to true.
- I would suggest adding type safety to the handleEnterDown function by adding a type check for the KeyboardEvent parameter.
- Finally, I would suggest adding a comment to the handleEnterDown function explaining what it does.
@@ -47,6 +48,7 @@ export default defineComponent({ | |||
value={inputValue.value} | |||
onCompositionstart={handleCompositionStart} | |||
onCompositionend={handleCompositionEnd} | |||
onKeydown={handleEnterDown} | |||
onInput={handleInput} | |||
/> | |||
{multiple && <span ref={mirrorRef} class={`${prefixCls}-mirror`} aria-hidden></span>} |
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.
from the code review.
-
The handleEnterDown event handler has been added but it is not clear what it does. It should be clearly specified in the comments or else it should not be added.
-
The onKeydown event listener has been added to the input but it is unclear what it will do when the enter key is pressed. It should be specified in the comments or the code should be modified to make its purpose more evident.
-
The multiple variable is not used in the code and should be removed.
-
The code formatting should be improved to make it easier to read and understand. Variables and functions should be declared in a consistent manner and the code should follow a logical structure.
-
Comments should be added to explain what the code is doing and why it is doing it. This will improve the readability and maintainability of the code.
Overall, this code patch looks good and should pass a basic code review. However, there are some areas that could use improvement such as adding comments, improving code formatting, and removing unused variables.
@@ -22,6 +22,7 @@ export interface SelectorContext { | |||
handleCompositionStart: (evt: CompositionEvent) => void | |||
handleCompositionEnd: (evt: CompositionEvent) => void | |||
handleInput: (evt: Event) => void | |||
handleEnterDown: (evt: KeyboardEvent) => void | |||
} | |||
|
|||
export const selectorToken: InjectionKey<SelectorContext> = Symbol('selectorToken') |
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 the code review now:
-
The code looks good and complete. There is an additional handleEnterDown function that has been added to the SelectorContext interface. This function will be triggered when the user presses the Enter key.
-
There are no visible bugs in the code and no improvement suggestions. However, you should make sure that the function is properly tested before deploying it to production.
Codecov Report
@@ Coverage Diff @@
## main #1513 +/- ##
=======================================
Coverage 92.75% 92.75%
=======================================
Files 331 331
Lines 30801 30804 +3
Branches 3533 3534 +1
=======================================
+ Hits 28568 28571 +3
Misses 2233 2233
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -116,5 +124,6 @@ export function useInputState(props: SelectorProps, mergedSearchable: ComputedRe | |||
handleCompositionEnd, | |||
handleInput, | |||
clearInput, | |||
handleEnterDown, | |||
} | |||
} |
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.
the code review
- This code looks good overall, with the exception of a few minor issues.
- The handleEnterDown function is missing a 'keyCode' check to make sure it is only triggered when the enter key is pressed.
- The clearInput function should be made more robust to make sure that it can handle all types of inputs.
- The handleInput function should also be made more robust by adding checks for other types of input events such as mouse and keyboard events.
- The useInputState function could benefit from better variable and function naming for better readability.
- The handleCompositionEnd function should include a try/catch block to handle any errors that may occur.
- The handleInput function should also include a try/catch block in case any errors are thrown.
2a93dfe
to
f2ce0cd
Compare
@@ -137,6 +138,7 @@ export default defineComponent({ | |||
handleCompositionStart, | |||
handleCompositionEnd, | |||
handleInput, | |||
handleEnterDown, | |||
}) | |||
|
|||
const elementRef = ref<HTMLDivElement>() |
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 the code review:
The code patch looks to be adding a handleEnterDown function to the useInputState hook. This will allow for the user to press enter and have the form submit, which is a welcome feature. However, there could be potential risks associated with this new functionality, such as unintended form submission. It would be a good idea to add additional tests to ensure that the handleEnterDown function works as expected. Additionally, there may be a need to update the documentation for the useInputState hook to reflect the new handleEnterDown function.
@@ -116,5 +124,6 @@ export function useInputState(props: SelectorProps, mergedSearchable: ComputedRe | |||
handleCompositionEnd, | |||
handleInput, | |||
clearInput, | |||
handleEnterDown, | |||
} | |||
} |
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.
code review
-
The code looks correct, but there is a potential bug risk when handleEnterDown function is triggered. It should check for evt.key instead of evt.code.
-
The handleEnterDown function should also call the callEmit function to emit the onInput event.
-
For improved readability, the handleEnterDown function should be moved above the handleInput function.
-
Add comments to explain what the handleEnterDown function is used for.
@@ -47,6 +48,7 @@ export default defineComponent({ | |||
value={inputValue.value} | |||
onCompositionstart={handleCompositionStart} | |||
onCompositionend={handleCompositionEnd} | |||
onKeydown={handleEnterDown} | |||
onInput={handleInput} | |||
/> | |||
{multiple && <span ref={mirrorRef} class={`${prefixCls}-mirror`} aria-hidden></span>} |
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 a brief code review:
The code patch adds the handleEnterDown function to the list of functions in the inject() call, and adds an onKeydown event listener to the input element. This should work as expected, but it is important to note that the onKeydown event listener should have the correct logic to handle the Enter key press properly. It is also important to make sure that the handleEnterDown function is correctly defined and does not introduce any potential bugs or security risks.
@@ -22,6 +22,7 @@ export interface SelectorContext { | |||
handleCompositionStart: (evt: CompositionEvent) => void | |||
handleCompositionEnd: (evt: CompositionEvent) => void | |||
handleInput: (evt: Event) => void | |||
handleEnterDown: (evt: KeyboardEvent) => void | |||
} | |||
|
|||
export const selectorToken: InjectionKey<SelectorContext> = Symbol('selectorToken') |
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.
:
This code patch adds a new handleEnterDown() function to the SelectorContext interface. This will allow users to handle KeyboardEvents when they press the enter key. This feature could be useful in certain applications, such as form creation. However, it is important to make sure that the code is secure and free of any potential bugs.
First, it is important to ensure that the input event is properly handled. A thorough check should be done to make sure that no malicious data can be passed into the handleInput() function. Additionally, proper validation should be done to make sure that the data contained in the KeyboardEvent is valid and does not contain any unexpected values.
Second, it is important to make sure that the handleEnterDown() function is properly implemented. This function should be tested thoroughly before being released to make sure that it triggers the expected behavior for all use cases. The code should also be reviewed for any potential security risks, such as cross-site scripting (XSS) attacks.
Finally, it is important to consider any potential performance issues that may arise from the addition of this code. It may be beneficial to add logging and metrics to track the performance of this feature. This will help identify any potential bottlenecks or areas of improvement.
Overall, this code patch appears to be a beneficial addition, but it is important to ensure that all security and performance concerns have been addressed.
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.
先临时这么解决把,理论上回车键会出触发 onCompositionend
才对。
我简单跟踪了下代码,是因为 clearInput 被调用导致的。
PR Checklist
Please check if your PR fulfills the following requirements:
What is the current behavior?
fix #1474
![image](https://user-images.githubusercontent.com/82451257/227596463-6e00bc83-ddde-47b9-880e-9bc2f30ac2a1.png)
input 添加了 compostionStart 事件和 compositionEnd 事件,但是由于中文输入法的原因 compositionEnd 事件不能被正常触发
就会导致 isComposing 一直为 true,从而下面的 inputValue change 等一系列事件不会运行。
What is the new behavior?
Other information