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

feat(pro:search): add customNameLabel and customOperatorLabel #1491

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

sallerli1
Copy link
Contributor

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?

What is the new behavior?

新增 customNameLabelsearchField.customOperatorLabel ,用来自定义 name 和 操作符 下拉选择框 label

Other information

@idux-bot
Copy link

idux-bot bot commented Mar 6, 2023

This preview will be available after the AzureCI is passed.

## en

Customize label of `name` select panel via `customNameLabel`.
Customize label of `operator` select panel via `searchField.customOperatorLabel`.
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 review!

This patch appears to be for an English-Chinese translation of a user interface element. The code looks accurate and free of errors, so it should be ready for implementation. Thus, from a code review perspective, the patch looks good. However, it could be improved in terms of the user experience. For example, if there is space, it might be helpful to include some more detailed instructions on how to use the custom labels. Additionally, if the labels are related to other user interface elements, it might be helpful to provide a visual representation of what they are and how they work. Finally, it might be helpful to include a description of any potential risks or issues that could arise from using this feature.

}
</script>

<style scoped lang="less"></style>
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. Variable names should be meaningful and avoid using abbreviations.
  2. The code should follow the conventions of VueJS like using kebab-case for props, camelCase for data attributes, etc.
  3. On line 8, the IxTooltip component should have a unique id attribute to support accessibility.
  4. Check if the code is correctly indented.
  5. Add comments to explain the purpose of each section of the code.
  6. Update the script setup on line 19 to include the types of data that are being passed in as arguments.
  7. Check if there is any redundant or unused code.
  8. Make sure that the code is optimized for readability and performance.
  9. Check if the code has been tested for possible errors and exceptions.

@@ -65,6 +66,7 @@ interface SearchItemConfirmContext<V = unknown> extends Partial<SearchValue<V>>
| `operators` | 搜索条件的中间操作符 | `string[]` | - | - | 提供时,会在搜索词条名称中间增加一个操作符,如 `'='`, `'!='` |
| `defaultOperator` | 默认的操作符 | `string` | - | - | 提供时,会自动填入默认的操作符 |
| `defaultValue` | 默认值 | - | - | - | 提供时,会自动填入默认值 |
| `customOperatorLabel` | 自定义操作符下拉选择label | `string \| ((operator: string) => VNodeChild)` | - | - | - |
| `inputClassName` | 输入框class | `string` | - | - | 用于自定义输入框样式 |
| `placeholder` | 输入框placeholder | `string` | - | - | 搜索值输入框的占位符 |
| `validator` | 搜索项校验函数 | `(value: SearchValue) => { message?: string } | undefined` | - | - | 返回错误信息 |
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 patch above

  1. clearIcon: clearly specify the type of data that it can accept for better understanding and coding.
  2. customNameLabel: it's a good practice to have a default value for this parameter so that users don't have to set it every time they use this component.
  3. defaultOperator: it's a good practice to have a default value for this parameter as well.
  4. customOperatorLabel: same as customNameLabel, it's a good practice to have a default value for this parameter.
  5. inputClassName: it's better to set the parameter type to string | undefined instead of string.
  6. validator: it's better to define the return type of the validator.

These are just some of the suggestions I have. I hope this helps.

@@ -180,7 +179,7 @@ export default defineComponent({
/>
<div class={`${prefixCls}-search-item-container`} style={searchItemContainerStyle.value}>
{searchItems.value?.map(item => (
<SearchItemComp key={item.key} searchItem={item} />
<SearchItemComp key={item.key} v-slots={slots} searchItem={item} />
))}
</div>
{searchValueEmpty.value && !activeSegment.value && (
Copy link

Choose a reason for hiding this comment

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

with a general overview of the code patch:

The code patch is for an export default defineComponent function, which provides props, locale, mergedPrefixCls, commonOverlayProps, etc. for a proSearchContext. In line 180, it also adds v-slots to the SearchItemComp component, which will be used to render search items. Finally, it adds a condition to display searchValueEmpty and activeSegment.

searchFields,
props.customNameLabel,
!operatorSegment,
)

return {
key: searchState.key,
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 overview of the code patch above. The code patch adds an extra parameter, customNameLabel, to the createNameSegment function. The customNameLabel is intended to allow for customizing the label that appears when searching for items. The code also adds an extra check to ensure that the operator segment is not present when creating the name segment.

From a code review perspective, the code looks correct and there are no obvious bugs or issues. However, as this is a new feature, it would be a good idea to add some basic unit tests to ensure that the code works as expected. Additionally, if possible, it would be helpful to add some documentation to explain what the customNameLabel parameter is used for and how it should be used.

@@ -161,7 +161,7 @@ export default defineComponent({
return (
<div class={prefixCls} tabindex={-1} onMousedown={evt => evt.preventDefault()}>
{renderSelectAll()}
<IxSelectPanel ref={panelRef} class={`${prefixCls}-inner`} {...panelProps} />
<IxSelectPanel ref={panelRef} class={`${prefixCls}-inner`} v-slots={slots} {...panelProps} />
{renderFooter()}
</div>
)
Copy link

Choose a reason for hiding this comment

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

with the first part of the code:

@@ -29,7 +29,7 @@ import { filterDataSource, matchRule } from '../utils/selectData'

export default defineComponent({
props: proSearchSelectPanelProps,

  • setup(props) {
  • setup(props, { slots }) {

This part of the code adds the 'slots' parameter to the setup function, allowing the component to access slots in the template. This should not introduce any bugs and should improve the code's readability.

The second part of the code:

@@ -161,7 +161,7 @@ export default defineComponent({
return (
<div class={prefixCls} tabindex={-1} onMousedown={evt => evt.preventDefault()}>
{renderSelectAll()}

  •      <IxSelectPanel ref={panelRef} class={`${prefixCls}-inner`} {...panelProps} />
    
  •      <IxSelectPanel ref={panelRef} class={`${prefixCls}-inner`} v-slots={slots} {...panelProps} />
         {renderFooter()}
       </div>
     )
    

This part of the code adds the 'v-slots' attribute to the IxSelectPanel component, allowing it to access any slots passed in by the parent template. This should also not introduce any bugs and should also improve the code's readability.

@@ -63,6 +63,7 @@ export default defineComponent({
{segmentRenderDatas.value.map(segment => (
<Segment
key={segment.name}
v-slots={slots}
itemKey={props.searchItem!.key}
input={segment.input}
value={segment.value}
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 review

The code looks well-formatted and easy to read. There are no obvious bugs or problems, but there are a few improvements that could be made.

Firstly, the setup function has been modified to include slots. By including slots, the component will be able to receive additional data from outside components. This could be useful for passing contextual data which can then be used within the component.

Secondly, the Segment component has been modified to include a v-slots property. This will allow the component to receive data from outside components, making it more versatile and dynamic.

Finally, the props have been moved to the top of the file, making them easier to access. This could help improve readability and maintainability of the code.

Overall, the code looks good, and the changes made should improve its functionality.

@@ -189,6 +189,7 @@ export default defineComponent({

const renderContent = () => {
const contentNode = props.segment.panelRenderer?.({
slots,
input: props.input ?? '',
value: props.value,
cancel: handleCancel,
Copy link

Choose a reason for hiding this comment

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

by looking at the code changes first.
The first change is on line 29, where the setup function now takes a second parameter called slots. This is likely to allow the panelRenderer function to access the slots defined in the component. This is a necessary change, as slots are not available in the props object.

The second change is on line 189, where the panelRenderer function is now passed the slots parameter. This allows the panelRenderer to access the slots defined in the component and thus render the desired content.

Overall, these seem to be valid changes and should not introduce any bugs. However, it is always good to do some additional testing to make sure everything works as expected.

return (
<SelectPanel
class={`${prefixCls}-name-segment-panel`}
v-slots={panelSlots}
value={convertArray(value)}
dataSource={filteredDataSource}
multiple={false}
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 review:

The code looks good overall. It looks like it is adding functionality to the existing code, so no major changes are needed. However, there are some minor issues that could be improved.

First, the import statement for VNodeChild should be added at the top of the file, instead of in the middle. Secondly, the variable name "customNameLabel" could be improved, as it is not very descriptive. Finally, the variable "_customNameLabel" should be renamed as it is not clear what it is used for.

In general, the code looks good and is ready for use.

@@ -41,7 +54,7 @@ export function createOperatorSegment(
return {
name: 'operator',
inputClassName: `${prefixCls}-operator-segment-input`,
defaultValue: searchField.operators.find(op => op === searchField.defaultOperator),
defaultValue: operators.find(op => op === defaultOperator),
parse: input => parseInput(input, searchField),
format: value => value ?? '',
panelRenderer,
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:

Overall, this code patch looks good. It adds a new import type, SelectPanelData, and updates the existing panelRenderer function to include an additional panelSlot. The code is also well commented and easy to read.

However, there are a few potential risks worth noting. There is no input validation for the customOperatorLabel, which may result in unexpected behavior. Additionally, the parseInput function is not defined, so it is unclear what kind of data is being passed into the function.

To improve the code, you could add some input validation to the customOperatorLabel to ensure that only valid data is being passed in. Additionally, you could add a definition of the parseInput function to make the code more readable.


export interface ProSearchContext extends SearchStateContext, ActiveSegmentContext, SearchTriggerContext {
props: ProSearchProps
slots: Slots
locale: ProSearchLocale
mergedPrefixCls: ComputedRef<string>
commonOverlayProps: ComputedRef<ɵOverlayProps>
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. The code looks well formatted and easy to read.
  2. The import statement for type Slots has been removed, so it is important to check if any of its usage is still in the code.
  3. It would be good to add a comment before each variable or function definition to explain their purpose.
  4. It would also be helpful to provide comments to explain the logic behind certain pieces of code.
  5. It would be beneficial to use consistent naming conventions throughout the code.
  6. It would be beneficial to add tests for the code to ensure its functionality.

@@ -19,6 +19,7 @@ export const proSearchProps = {
default: undefined,
},
clearIcon: [String, Object] as PropType<string | VNode>,
customNameLabel: [String, Function] as PropType<string | ((searchField: SearchField) => VNodeChild)>,
maxLabel: { type: [Number, String] as PropType<number | 'responsive'>, default: 'responsive' },
searchIcon: [String, Object] as PropType<string | VNode>,
disabled: {
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

First, let's look at the imports:
-import type { DefineComponent, HTMLAttributes, PropType, VNode } from 'vue'
+import type { DefineComponent, HTMLAttributes, PropType, VNode, VNodeChild } from 'vue'

The second import is better as it includes the VNodeChild type, which is required for the customNameLabel prop. This will help ensure type safety when using the customNameLabel prop.

Next, let's look at the props:
export const proSearchProps = {
value: Array as PropType<SearchValue[]>,
searchFields: Array as PropType<SearchField[]>,
disabled: {
type: Boolean as PropType,
default: false,
},
clearable: {
type: Boolean as PropType,
default: true,
},
onConfirm: Function as PropType<(context: SearchItemConfirmContext) => void>,
onError: Function as PropType<(error: SearchItemError) => void>,
popupContainer: Object as PropType,
clearIcon: [String, Object] as PropType<string | VNode>,

  • customNameLabel: [String, Function] as PropType<string | ((searchField: SearchField) => VNodeChild)>,
    maxLabel: {
    type: [Number, String] as PropType<number | 'responsive'>,
    default: 'responsive'
    },
    searchIcon: [String, Object] as PropType<string | VNode>,
    disabled: {
    type: Boolean as PropType,
    default: false,
    },
    }

It looks like the clearIcon and searchIcon props are redundant, as they are both of type [String, Object] as PropType<string | VNode>. We can remove one of them to reduce clutter.

We can also improve the readability of the props by adding line breaks between each prop and adding comments to explain what each prop does.

Finally, there is an issue with the disabled prop - it is defined twice. We should remove the second definition of disabled and ensure that the correct value is set for this prop.

Overall, the patch looks good and the changes made should improve type safety and readability.

@@ -26,6 +26,7 @@ interface SearchFieldBase<V = unknown> {
defaultValue?: V
inputClassName?: string
placeholder?: string
customOperatorLabel?: string | ((operator: string) => VNodeChild)
validator?: (value: SearchValue<V>) => Omit<SearchItemError, 'index'> | undefined
onPanelVisibleChange?: (visible: boolean) => void
}
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 solid. There are no obvious bugs or issues. It seems like the customOperatorLabel property was added to the SearchFieldBase interface in order to allow for a custom label for the operator field. This is a good addition and should not cause any problems. The validator property also looks good, ensuring that the search value is valid before it is accepted.

As far as improvement suggestions, it would be helpful to include a description of the customOperatorLabel property in the code, such as what type of input it accepts, what it does, and what type of output it produces. This will help other developers understand how to use the new property and make working with it easier.

Overall, the code looks good and should work as expected.


export type InputFormater<V = unknown> = (value: V) => string
export type InputParser<V = unknown> = (input: string) => V | null

export interface PanelRenderContext<V = unknown> {
slots: Slots
input: string
value: V
ok: () => void
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 looks generally good, there is no obvious bug risk. However, there is one potential improvement that can be made. The "PanelRenderContext" interface should include a type for the "slots" property so that it is more clear what this property should contain. By adding "Slots" type to the property, it will make it easier to understand its purpose and use.

@sallerli1 sallerli1 force-pushed the feat-pro-search-name-renderer branch from 3f412e4 to ca4d79f Compare March 6, 2023 12:47
## en

Customize label of `name` select panel via `customNameLabel`.
Customize label of `operator` select panel via `searchField.customOperatorLabel`.
Copy link

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 good. I cannot see any bug risks in the code.

However, there are a few suggestions that can be made for improvement:

  1. The code should use consistent indentation.
  2. The code should be formatted for better readability.
  3. The code should use meaningful variable names.
  4. Comments should be added to explain what the code does.

}
</script>

<style scoped lang="less"></style>
Copy link

Choose a reason for hiding this comment

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

with a code review of the above code:

  1. The code appears to be using Vue components and typescript setup. It looks like it is setting up an IxProSearch component and passing props such as searchFields, onChange, and onSearch.

  2. The code does not appear to have any bugs or errors, however there are a few improvements that could be made. For example, it might be beneficial to add comments to the code to explain what certain sections of the code are doing. This would make it easier for someone else to understand the code. Additionally, it might also be beneficial to refactor the code in order to make it more efficient and readable.

@@ -65,6 +66,7 @@ interface SearchItemConfirmContext<V = unknown> extends Partial<SearchValue<V>>
| `operators` | 搜索条件的中间操作符 | `string[]` | - | - | 提供时,会在搜索词条名称中间增加一个操作符,如 `'='`, `'!='` |
| `defaultOperator` | 默认的操作符 | `string` | - | - | 提供时,会自动填入默认的操作符 |
| `defaultValue` | 默认值 | - | - | - | 提供时,会自动填入默认值 |
| `customOperatorLabel` | 自定义操作符下拉选择label | `string \| ((operator: string) => VNodeChild)` | - | - | - |
| `inputClassName` | 输入框class | `string` | - | - | 用于自定义输入框样式 |
| `placeholder` | 输入框placeholder | `string` | - | - | 搜索值输入框的占位符 |
| `validator` | 搜索项校验函数 | `(value: SearchValue) => { message?: string } | undefined` | - | - | 返回错误信息 |
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. It is important to ensure that the added parameter customNameLabel and customOperatorLabel have valid default values or are not required parameters, so that they can be used properly when they are not set by users. If they are optional parameters, then the necessary validation should be added to ensure that the values provided by users are valid.

  2. Ensure that the input provided by the user is correctly sanitized before being used in the code.

  3. Make sure that all error messages are descriptive and easy to understand, so that users can easily identify the issues.

  4. Make sure that the code is well documented, so that it is easy for other developers to understand and modify.

  5. Make sure that the code is tested thoroughly for any bugs or vulnerabilities before being deployed.

@@ -184,7 +183,7 @@ export default defineComponent({
/>
<div class={`${prefixCls}-search-item-container`} style={searchItemContainerStyle.value}>
{searchItems.value?.map(item => (
<SearchItemComp key={item.key} searchItem={item} />
<SearchItemComp key={item.key} v-slots={slots} searchItem={item} />
))}
</div>
{searchValueEmpty.value && !activeSegment.value && (
Copy link

Choose a reason for hiding this comment

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

with a basic review of the code patch

First, we can see that a line of code has been removed (line 134) which was providing slots to the SearchItemComp component. This is not ideal as it could cause errors down the line if the slots are needed for some functionality.

Second, we can see that a line of code has been added (line 183) which provides the slots to the SearchItemComp component. This is good as it prevents any errors caused by missing slots.

Overall, this code patch looks good but it would be good to make sure that the slots are actually needed and that they provide the desired functionality.

searchFields,
props.customNameLabel,
!operatorSegment,
)

return {
key: searchState.key,
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:

This code looks to be making some changes to the useSearchItems() function which is used for search. The patch adds a new parameter, props.customNameLabel, to the createNameSegment() function which is called within the useSearchItems() function.

The code looks correct and should not introduce any bug risks. However, it is important to ensure that the customNameLabel being passed into the createNameSegment() function is of the correct type, as this could potentially lead to an error.

In terms of improvement suggestions, it might be beneficial to add in more comments or documentation to the code to explain what the purpose of the customNameLabel parameter is and why it is being used. This would make it easier for other developers to understand the code and debug it if necessary.

@@ -159,7 +159,7 @@ export default defineComponent({
return (
<div class={prefixCls} tabindex={-1} onMousedown={evt => evt.preventDefault()}>
{renderSelectAll()}
<IxSelectPanel ref={panelRef} class={`${prefixCls}-inner`} {...panelProps} />
<IxSelectPanel ref={panelRef} class={`${prefixCls}-inner`} v-slots={slots} {...panelProps} />
{renderFooter()}
</div>
)
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. In the setup method, props should be destructured instead of passed in directly.
  2. The v-slots should be passed into the IxSelectPanel component.
  3. The renderSelectAll() method should be called before the IxSelectPanel component, so that the select all checkbox is always at the top.
  4. The computed function should be memoized for better performance.
  5. The activeValue state should be defined as a ref instead of a useState hook.
  6. Consider using the useEffect hook to handle side effects such as setting the focus on the SelectPanel when it is opened.
  7. Add type annotations to your props and variables.
  8. Add comments to explain complex sections of your code.

@@ -63,6 +63,7 @@ export default defineComponent({
{segmentRenderDatas.value.map(segment => (
<Segment
key={segment.name}
v-slots={slots}
itemKey={props.searchItem!.key}
input={segment.input}
value={segment.value}
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 first change to the code is setting up a new argument in the setup function of the component called 'slots'. It appears that this will be used to pass in any custom slots that have been declared for the component, which is a good way to provide the user with more flexibility in the customization of the component.

  2. The second change adds an additional attribute to the Segment component, v-slots, which is used to pass in the custom slots declared for the component. This is also a good addition, as it allows for further customization of the component.

In general, these changes appear to be designed to give the user more control over the customization of the component, and should improve the overall usability of the component. If there are no other issues, these changes should be accepted.

@@ -189,6 +189,7 @@ export default defineComponent({

const renderContent = () => {
const contentNode = props.segment.panelRenderer?.({
slots,
input: props.input ?? '',
value: props.value,
cancel: handleCancel,
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:

This code patch looks correct and should work properly. However, there are a few improvements that could be made to ensure optimal performance and readability.

First, it would be beneficial to add type annotations to the variables that are used in the setup function. This will help the compiler better understand the types of data being used, allowing it to generate more efficient code.

Second, it would be beneficial to refactor the renderContent function to break it up into smaller, more concise functions that can be easily read. This will make it easier to follow the logic and identify potential bugs.

Finally, it would be helpful to add comments to explain what each line of code is doing and why it is necessary. This will make it easier for other developers to quickly understand the code and make any necessary changes.

@@ -211,7 +209,6 @@ function getKeyByLabels(
keys.unshift((currentKey = parentKeyMap.get(currentKey)!))
}

console.log('getKeys', keys)
return keys
}

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. In the first section, in line 88, the strategy property should be changed from cascaderStrategy to mergedCascaderStrategy. This is likely an improvement for performance, as it is more efficient to use a merged strategy than individual strategies.

  2. In the second section, lines 197-211, the code appears to be attempting to resolve checked keys. However, the two console.log() statements have been removed. It is recommended that these are added back in so that any issues with the code can be identified and fixed. Additionally, it would be beneficial to add comments to explain what is happening in this section of code.

return (
<SelectPanel
class={`${prefixCls}-name-segment-panel`}
v-slots={panelSlots}
value={convertArray(value)}
dataSource={filteredDataSource}
multiple={false}
Copy link

Choose a reason for hiding this comment

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

with the first few lines of code

The code imports several types from the "../types" module, and also imports the lodash-es library. This is good practice, as it allows you to access the types and utility functions you need without having to write them all out yourself.

The code then creates a function called createNameSegment, which takes in several parameters such as prefixCls, searchFields, customNameLabel, and showEndSymbol. This function is responsible for creating a segment that allows users to select a name from a list of available options.

The function also has a panelRenderer which is responsible for handling user input and setting the value of the selected option. It also contains logic for handling key inputs and setting the onKeyDown event.

The code also includes a renderNameLabel function which is used to render the custom label for the selected option. Finally, the code includes a SelectPanel component which is used to render the list of available options.

@@ -41,7 +54,7 @@ export function createOperatorSegment(
return {
name: 'operator',
inputClassName: `${prefixCls}-operator-segment-input`,
defaultValue: searchField.operators.find(op => op === searchField.defaultOperator),
defaultValue: operators.find(op => op === defaultOperator),
parse: input => parseInput(input, searchField),
format: value => value ?? '',
panelRenderer,
Copy link

Choose a reason for hiding this comment

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

to review the code
The code is trying to create an operator segment which consists of a SelectPanel. It looks good and is properly indented and commented.

The code is using lodash-es for checking if a value is a string. However, it's not necessary to use a library for this when javascript has built-in functions like typeof and instanceof.

The code is using the default values for the searchField parameters in the defaultValue of the return object but this should be done outside of that function.

The code is using setOnKeyDown but it's not clear how this is being used. This should be more clearly explained in the comments or code.

Overall, the code looks good and there are no major issues. Some minor changes should be made to improve readability and performance.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #1491 (882c785) into main (1cd4dcc) will increase coverage by 0.00%.
The diff coverage is 50.00%.

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

@@           Coverage Diff           @@
##             main    #1491   +/-   ##
=======================================
  Coverage   92.73%   92.73%           
=======================================
  Files         331      331           
  Lines       30754    30752    -2     
  Branches     3533     3533           
=======================================
- Hits        28520    28519    -1     
+ Misses       2234     2233    -1     
Impacted Files Coverage Δ
packages/pro/search/src/ProSearch.tsx 16.35% <0.00%> (+0.07%) ⬆️
packages/pro/search/src/token.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.


export interface ProSearchContext
extends SearchStateContext,
SearchStateWatcherContext,
ActiveSegmentContext,
SearchTriggerContext {
props: ProSearchProps
slots: Slots
locale: ProSearchLocale
mergedPrefixCls: ComputedRef<string>
commonOverlayProps: ComputedRef<ɵOverlayProps>
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 well structured, and the indentation is correct.
  2. The variable names are descriptive and understandable.
  3. The code is free from syntax errors.
  4. The code is free of any potential bugs or security risks.
  5. The import statements are organized properly.
  6. The code is easily readable and understandable.
  7. The types of variables are properly declared and used.
  8. The code is efficiently implemented.
  9. The code follows the best coding practices.
  10. There is no redundant code present in the code patch.

@@ -19,6 +19,7 @@ export const proSearchProps = {
default: undefined,
},
clearIcon: [String, Object] as PropType<string | VNode>,
customNameLabel: [String, Function] as PropType<string | ((searchField: SearchField) => VNodeChild)>,
maxLabel: { type: [Number, String] as PropType<number | 'responsive'>, default: 'responsive' },
searchIcon: [String, Object] as PropType<string | VNode>,
disabled: {
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,

  1. the import type is not necessary, since the types are already imported in the previous line.
  2. The proSearchProps should be constant and use PascalCase for variable name.
  3. The PropType for customNameLabel should be changed to [String, Function] as PropType<string | ((searchField: SearchField) => VNodeChild)>
  4. The maxLabel prop should be optional and have a default value.
  5. The disabled prop should be optional and have a default value.
  6. The clearIcon prop should be optional and have a default value.
  7. The searchIcon prop should be optional and have a default value.

@@ -26,6 +26,7 @@ interface SearchFieldBase<V = unknown> {
defaultValue?: V
inputClassName?: string
placeholder?: string
customOperatorLabel?: string | ((operator: string) => VNodeChild)
validator?: (value: SearchValue<V>) => Omit<SearchItemError, 'index'> | undefined
onPanelVisibleChange?: (visible: boolean) => void
}
Copy link

Choose a reason for hiding this comment

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

This patch looks good, there are no obvious bug risks. I recommend a few improvements such as adding additional validation check for the customOperatorLabel field, to ensure that only valid strings or callbacks are accepted. Additionally, you can add documentation to the code to make it clear what this field is used for. This will help future developers who work on this code.

Thanks!


export type InputFormater<V = unknown> = (value: V) => string
export type InputParser<V = unknown> = (input: string) => V | null

export interface PanelRenderContext<V = unknown> {
slots: Slots
input: string
value: V
ok: () => void
Copy link

Choose a reason for hiding this comment

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

  1. The code looks good and there are no syntactical errors.

  2. It is however advisable to use meaningful variable names for e.g, in the interface PanelRenderContext, it would be better to use 'slotProps' instead of slots. This will help other developers understand the code easily.

  3. The imports should be in alphabetical order for easier readability and understanding.

  4. Use comments to explain the code if needed. This will help other developers understand the code quickly.

@danranVm danranVm merged commit 1be87e8 into IDuxFE:main Mar 6, 2023
@sallerli1 sallerli1 deleted the feat-pro-search-name-renderer branch July 4, 2024 09:27
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.

2 participants