Skip to content
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

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

kovsu
Copy link
Member

@kovsu kovsu commented Mar 24, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added/updated or not needed
  • Docs and demo have been added/updated or not needed

What is the current behavior?

fix #1474
input 添加了 compostionStart 事件和 compositionEnd 事件,但是由于中文输入法的原因 compositionEnd 事件不能被正常触发
image
就会导致 isComposing 一直为 true,从而下面的 inputValue change 等一系列事件不会运行。

What is the new behavior?

Other information

@@ -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>}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review:

  1. The code looks correct and is properly indented.
  2. The handleKeyDown function is correctly defined and uses the correct event type for the KeyboardEvent.
  3. The onKeyDown event handler is correctly attached to the input element.
  4. The event handler is correctly checking the event code property to detect if the enter key has been pressed.
  5. The handleCompositionEnd function is correctly called when the enter key is pressed.
  6. The readonly computed value looks correct and is properly used to control the input readonly attribute.
  7. 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.

@idux-bot
Copy link

idux-bot bot commented Mar 24, 2023

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>()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review

  1. 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.

  2. The handleEnterDown function should be tested to make sure that it correctly handles user input, such as when the user presses the enter key.

  3. The getBoundingClientRect() function should be tested to make sure that it is returning the correct values.

  4. The use of the ref() should be verified to make sure that it is not creating any memory leaks.

  5. 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,
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review.

  1. From my point of view, the code looks clean and easy to understand.
  2. 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.
  3. 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.
  4. I would suggest adding type safety to the handleEnterDown function by adding a type check for the KeyboardEvent parameter.
  5. 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>}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the code review.

  1. 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.

  2. 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.

  3. The multiple variable is not used in the code and should be removed.

  4. 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.

  5. 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')
Copy link

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:

  1. 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.

  2. 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
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #1513 (a30e73e) into main (703122f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a30e73e differs from pull request most recent head f2ce0cd. Consider uploading reports for the commit f2ce0cd to get more accurate results

@@           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           
Impacted Files Coverage Δ
...ages/components/_private/selector/src/Selector.tsx 91.15% <100.00%> (+0.06%) ⬆️
packages/components/_private/selector/src/token.ts 100.00% <100.00%> (ø)
packages/components/button/src/Button.tsx 95.37% <100.00%> (ø)

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,
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review

  1. This code looks good overall, with the exception of a few minor issues.
  2. The handleEnterDown function is missing a 'keyCode' check to make sure it is only triggered when the enter key is pressed.
  3. The clearInput function should be made more robust to make sure that it can handle all types of inputs.
  4. The handleInput function should also be made more robust by adding checks for other types of input events such as mouse and keyboard events.
  5. The useInputState function could benefit from better variable and function naming for better readability.
  6. The handleCompositionEnd function should include a try/catch block to handle any errors that may occur.
  7. The handleInput function should also include a try/catch block in case any errors are thrown.

@@ -137,6 +138,7 @@ export default defineComponent({
handleCompositionStart,
handleCompositionEnd,
handleInput,
handleEnterDown,
})

const elementRef = ref<HTMLDivElement>()
Copy link

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,
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review

  1. 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.

  2. The handleEnterDown function should also call the callEmit function to emit the onInput event.

  3. For improved readability, the handleEnterDown function should be moved above the handleInput function.

  4. 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>}
Copy link

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')
Copy link

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.

Copy link
Member

@danranVm danranVm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

先临时这么解决把,理论上回车键会出触发 onCompositionend 才对。

我简单跟踪了下代码,是因为 clearInput 被调用导致的。

@danranVm danranVm merged commit 33a2cf6 into IDuxFE:main Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[comp:select]IxSelect在中文输入法输入时回车,onSearch事件会失效
2 participants