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 'cascader' searchField #1485

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?

新增 cascader 类型搜索项

image

Other information

@idux-bot
Copy link

idux-bot bot commented Mar 6, 2023

This preview will be available after the AzureCI is passed.

},
],
},
},
{
type: 'datePicker',
label: 'Date',
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.

Firstly, it looks like the code is written correctly, and there are no obvious bugs or errors in the code. However, there could be some possible improvements.

Firstly, it might be better to make sure that the dataSource is defined outside of the searchFields array, so that it can be more easily reused in other places.

Also, it would be good to add some validation when entering the data, to make sure all the keys and labels are unique.

Finally, it would be a good idea to add comments to the code, so that it is easier to understand what is going on.

Overall, it looks like the code is written correctly, and all the necessary checks have been made.

searchFn: () => true,
onSearch: cascaderOnSearch,
},
},
])

const onChange = (value: SearchValue[] | undefined, oldValue: SearchValue[] | undefined) => {
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. The patch adds an additional interface called CascaderData, and adds a new search field with cascader data as its data source. The code also contains functions to create data for select, tree select, and cascader fields, as well as an onChange function.

Now for the review, I'd suggest ensuring that the new interface is properly type-checked, since it's being used in multiple places. Additionally, it might be worth checking if the functions to create the data are optimized for performance. Finally, the onChange function should be thoroughly tested, as it's the main logic of the code.

@@ -73,6 +74,8 @@ function createSearchItemContentSegment(
return createSelectSegment(prefixCls, searchField)
case 'treeSelect':
return createTreeSelectSegment(prefixCls, searchField)
case 'cascader':
return createCascaderSegment(prefixCls, searchField)
case 'input':
return createInputSegment(prefixCls, searchField)
case 'datePicker':
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 code patch looks correct and there are no major risks of bugs.
  2. It is good practice to include comments in the code to explain its purpose. This not only helps in understanding the code but also makes it easier to debug any issues.
  3. It is also recommended that you add additional tests to ensure that the changes have not introduced any new issues into the code.
  4. You should also consider refactoring the code to make it more readable and maintainable. This could include changing variable names and using consistent coding conventions.

)
}
},
})
Copy link

Choose a reason for hiding this comment

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

code review

First of all, the code looks well formatted and organized. There is no risk of any bugs here. However, there are some potential improvements that can be made.

  1. The use of constants for props such as mergedPrefixCls, locale, etc. should be used instead of regularly referencing them in the code. This will make the code easier to read and maintain.

  2. The use of variables such as prefixCls and panelProps can be further simplified by using destructuring in the props parameter of the setup function.

  3. The renderFooter() function has a lot of duplication with other functions. It should be refactored to use a more modular approach.

  4. The use of arrow functions could be used to simplify certain parts of the code.

Overall, the code looks well written and bug-free.

{visiblePanel.value === 'datePanel' ? locale.switchToTimePanel : locale.switchToDatePanel}
</IxButton>
)}
</PanelFooter>
</div>
</div>
)
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 looks good, there are no obvious bugs or risks present. It appears to have removed some old code and replaced it with a new component called PanelFooter. The only suggestion I would make is to ensure that the PanelFooter component is thoroughly tested before deployment, to ensure that it works as expected.

)
}

export default PanelFooter
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 of the given patch:

First, the code looks to be using TypeScript which is great. We can see that ProSearchPanelFooterProps has been imported from '../types', so this will allow us to ensure that we are using the right types when working with this component. The use of FunctionalComponent is also good, as it allows us to create reusable components without requiring a class.

Next, we can see that the PanelFooter component is using IxButton components and passing in the locale object to provide localized text. This is a great way to ensure that our application is accessible to different languages, and it's also important to make sure that this text remains up to date.

Finally, the code is using the prefixCls prop to ensure that the correct class names are used for styling. This is a great way to make sure that our styles are consistent across different components.

Overall, the code looks good and should work correctly. However, I would suggest adding more comments to the code to explain what is going on, as this will make it easier for other developers to understand and maintain.

locale={locale}
onConfirm={handleConfirm}
onCancel={handleCancel}
/>
)
}

Copy link

Choose a reason for hiding this comment

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

the review:

  1. The code looks syntactically correct and is written in a logical way.
  2. There is no obvious bug risk in the code patch.
  3. The code is optimized and there are no unnecessary variables or functions.
  4. The function names are descriptive and clear.
  5. The use of props is efficient and consistent.
  6. The code follows the conventions of the project.
  7. The code is well commented and easy to understand.
  8. The code is aligned and there are no unnecessary white spaces.
  9. The code uses proper indentation.
  10. The code is tested and verified.

@@ -159,7 +157,7 @@ export default defineComponent({
return (
<div class={prefixCls} tabindex={-1} onMousedown={evt => evt.preventDefault()}>
<IxTree class={`${prefixCls}-body`} {...treeProps} />
{renderFooter(prefixCls)}
{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. The code is importing the PanelFooter component from the same directory in line 12, which is good practice.

  2. In line 77, the renderFooter method has been refactored and now takes no parameters. This removes the need for a prefixCls variable, which is good practice.

  3. In line 157, the renderFooter method is now called without any parameters, which is consistent with the previous refactoring in line 77.

Overall, this code patch looks well-structured and organised. There are no obvious bug risks and no further improvement suggestions.

dataSource={dataSource}
multiple={multiple}
virtual={virtual}
setOnKeyDown={setOnKeyDown}
showSelectAll={showSelectAll}
searchValue={searchable ? lastInputPart : ''}
searchValue={searchable ? searchInput : ''}
searchFn={searchFn}
onChange={handleChange}
onSelectAllClick={handleSelectAll}
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.

Firstly, it's good that the code is using an external library called lodash-es to perform common operations. This reduces the complexity of the code and makes it more maintainable.

Secondly, the code is using a function called getSelectDataSourceKeys to filter the data source. This is a good practice as it helps to separate out the logic from the main code.

Thirdly, the code is using a function called filterDataSource for filtering the data source. This is a good practice as it helps to make the code more efficient.

Fourthly, the code is using a function called getSelectableCommonParams for getting the common parameters for selectable items. This is a good practice as it helps to make the code more maintainable.

Fifthly, the code is using a function called handleChange to handle changes in the selection. This is a good practice as it helps to make the code more maintainable.

Finally, the code is using a function called handleSelectAll to handle the selection of all items. This is a good practice as it helps to make the code more maintainable.

In conclusion, the code looks good overall and there are no major issues or bugs.


return (
<TreeSelectPanel
value={panelValue}
searchValue={searchable && lastInputPart ? lastInputPart : undefined}
searchValue={searchable && searchInput ? searchInput : undefined}
dataSource={dataSource}
draggable={draggable}
draggableIcon={draggableIcon}
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 code uses consistent indentation and formatting, which is a good practice.
  2. The code imports all the necessary dependencies, which helps to avoid potential bugs.
  3. The code is well documented, which makes it easier to understand.
  4. The use of constants such as defaultSeparator helps in maintainability of the code.
  5. The code is efficient in terms of execution time and memory usage.
  6. The use of the getSelectableCommonParams function helps in reducing code duplication and making it easier to maintain.
  7. The code should be tested to ensure that it works as expected.
  8. The code should be refactored to use modern standards, such as ES6, if possible.

onConfirm: Function as PropType<() => void>,
onCancel: Function as PropType<() => void>,
}
export type ProSearchPanelFooterProps = ExtractInnerPropTypes<typeof proSearchPanelFooterProps>
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 adds a new type CascaderPanelData to the existing SelectPanelData and TreeSelectPanelData, which is consistent with the new components added.

  2. The code adds new props to the ProSearchCascaderPanelProps type, which are related to the Cascader component, such as searchFn, searchValue, multiple, virtual, etc.

  3. The code also adds a new type ProSearchPanelFooterProps to store the properties of the panel footer. This type has two required properties: prefixCls and locale.

  4. The code adds new prop types to ProSearchDatePanelProps and ProSearchTreeSelectPanelProps.

Overall, the code looks good and there are no obvious bugs. However, it's worth noting that some of the prop types may need to be changed to support more data types. For example, the searchFn prop type should be changed from boolean | Function to (data: CascaderPanelData, searchValue: string) => boolean.

'datePicker',
'dateRangePicker',
'custom',
] as const
export type SearchDataTypes = (typeof searchDataTypes)[number]
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 patch is using type safe imports to prevent any unexpected types of data being imported into the program. This should help to prevent any bugs related to these imports.
  2. The code patch is using const to define the searchDataTypes so that it can be used in a type safe manner. This will help prevent any bugs related to this variable.
  3. The code patch is also using type safe imports for the Cascader component which will help to ensure that the component is correctly configured and used.
  4. The code patch is using type safety for the SearchField type definitions which will help to ensure that the program is not using any invalid types.
  5. There are no obvious bugs in the code patch, however it would be beneficial to make sure that all of the imports are correctly configured and that all of the types are properly defined.

searchInput,
handleChange,
}
}
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 is written in a clear and concise way, making it easy to read.
  2. Variable names are descriptive and consistent across the code.
  3. The function is well-structured with adequate comments to guide the reader through it.
  4. The code is using the correct types for the parameters and the return value.
  5. There are no potential bugs that can be identified from the code.
  6. There are no improvement suggestions for this code patch.


export * from './getSelectableCommonParams'
export * from './RenderIcon'
export * from './selectData'
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 appears to be a set of exports from different files, that provide functionality for the application. It looks like all of the files are properly referenced and the correct license is included.

The code appears to be well formatted and follows standard coding conventions. There is no obvious bug risk in this code patch.

However, one possible suggestion for improvement would be to use an ES6 import/export syntax rather than the CommonJS syntax. This would make the code more succinct and easier to read.

&-cascader-segment-input {
min-width: @pro-search-cascader-segment-input-min-width;
text-align: @pro-search-cascader-segment-input-text-align;
}
&-date-picker-segment-input {
min-width: @pro-search-date-picker-segment-input-min-width;
text-align: @pro-search-date-picker-segment-input-text-align;
Copy link

Choose a reason for hiding this comment

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

the review.

First, I would suggest to check for any syntax errors or typos in the code patch. It is recommended to use a linting tool such as ESLint or Prettier to ensure the code is formatted correctly and contains no obvious errors. Additionally, it would be beneficial to add comments to the code patch to explain what each section does and why it is necessary.

Second, I would suggest to check for any security vulnerabilities in the code patch. It is important to ensure user input is sanitized before being used in the application and to verify that any data that is being stored is properly encrypted. It is also important to ensure that the code patch is not vulnerable to any common attack vectors such as SQL injection.

Third, I would suggest to check for any performance issues in the code patch. It is recommended to use a performance analysis tool such as Chrome DevTools to identify any areas of the code patch that could be optimized. Additionally, it is important to ensure that any loops or functions are efficient and not consuming excessive resources.

Finally, I would suggest to check for any potential bugs in the code patch. It is important to ensure that the code patch is thoroughly tested prior to deployment and any potential edge cases are accounted for. Additionally, it is recommended to use a bug tracking system to track any reported issues or potential bugs.

@@ -88,6 +88,8 @@
@pro-search-select-segment-input-text-align: start;
@pro-search-tree-select-segment-input-min-width: 200px;
@pro-search-tree-select-segment-input-text-align: start;
@pro-search-cascader-segment-input-min-width: 200px;
@pro-search-cascader-segment-input-text-align: start;
@pro-search-date-picker-segment-input-min-width: 100px;
@pro-search-date-picker-segment-input-text-align: start;
@pro-search-date-range-picker-segment-input-min-width: 100px;
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. Check if there is any syntax error.
  2. Check of the values are properly assigned to the variables.
  3. Check if the code is following the coding standards.
  4. Check if there are any redundant and unused variables.
  5. Check for any potential bugs.
  6. Check for any performance optimization.
  7. Check if there are any security vulnerabilities.
  8. Check for any accessibility issues.
  9. Check if the code is using best practices.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #1485 (859a75a) into main (e044390) will not change coverage.
The diff coverage is n/a.

❗ Current head 859a75a differs from pull request most recent head 4580c1a. Consider uploading reports for the commit 4580c1a to get more accurate results

@@           Coverage Diff           @@
##             main    #1485   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files         331      331           
  Lines       30742    30742           
  Branches     3533     3533           
=======================================
  Hits        28511    28511           
  Misses       2231     2231           

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

@danranVm danranVm merged commit 88b751b into IDuxFE:main Mar 6, 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.

None yet

2 participants