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.

Sign up for GitHub

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

Jump to bottom

fix(pro:search): name select label not rendering #1493

Merged
merged 1 commit into from
Mar 7, 2023
Merged

fix(pro:search): name select label not rendering #1493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/pro/search/demo/RemoteSearch.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ title:

## zh

`'select'`，`'treeSelect'` 类型搜索项支持服务端搜索。
`'select'`，`'treeSelect'`，`'cascader'` 类型搜索项支持服务端搜索。

## en

Server-side searching is supported under field type of `'select'`, `'treeSelect'`.
Server-side searching is supported under field type of `'select'`, `'treeSelect'`, `'cascader'`.
Copy link

Choose a reason for hiding this comment

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

the review:

The patch adds 'cascader' to the list of supported field types. This should provide no risk, but it might be worth double-checking that the server-side search is properly implemented for this new field type.

It would also be beneficial to provide an example of how to use this feature, as well as any potential limitations or caveats associated with its use.

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 patch seems to be adding support for server-side searching for 'cascader' field type. The code looks correct, but it is best to add a unit test to ensure that the code works as expected. Additionally, it may be beneficial to add a comment before the code change to explain why the change was made and the purpose of the code.

13 changes: 5 additions & 8 deletions packages/pro/search/src/segments/CreateNameSegment.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,18 @@ export function createNameSegment(
return
}

const renderNameLabel = (key: VKey, renderer?: (searchField: SearchField) => VNodeChild) => {
if (!renderer) {
return undefined
}

const renderNameLabel = (key: VKey, renderer: (searchField: SearchField) => VNodeChild) => {
const searchField = searchFields!.find(field => field.key === key)!
return renderer(searchField)
}

const _customNameLabel = customNameLabel ?? 'nameLabel'
const customNameLabelRender = isString(_customNameLabel) ? slots[_customNameLabel] : _customNameLabel

const panelSlots = {
optionLabel: isString(_customNameLabel)
? (option: SelectPanelData) => renderNameLabel(option.key, slots[_customNameLabel])
: (option: SelectPanelData) => renderNameLabel(option.key, _customNameLabel),
optionLabel: customNameLabelRender
? (option: SelectPanelData) => renderNameLabel(option.key, customNameLabelRender)
: undefined,
}

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 patch is adding a check for the existence of a slot before calling the renderNameLabel function, this is good practice and will make sure renderNameLabel does not get called with an invalid argument.

  2. The code patch can be improved by adding a check to make sure the _customNameLabel is valid. If it is not a string then it should not be passed to the renderNameLabel function.

  3. The code patch should also include a default value for the _customNameLabel in case it is not provided. This would prevent any errors from occurring if the _customNameLabel is undefined.

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 valid and does not contain any obvious errors.
  2. However, I would suggest that you add a type check for the _customNameLabel argument in the if statement. This will ensure that the correct data type is being passed to the function.
  3. Additionally, you might want to consider adding some logging or error handling code to handle unexpected values or errors resulting from the operation. This can help with debugging and make it easier to track down issues.

return (
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. It is not necessary to include the if (!renderer) { return undefined } statement in the renderNameLabel function as the parameter renderer is already given a type of (searchField: SearchField) => VNodeChild which means it can not be undefined.

  2. The constant _customNameLabel should be renamed to something more descriptive as it does not clearly describe its purpose.

  3. The customNameLabelRender constant should be initialized with the value of customNameLabel instead of _customNameLabel to ensure that the correct value is used.

  4. The optionLabel slot should be defined as a ternary expression to ensure that the customNameLabelRender is only used when it is truthy.

  5. The panelSlots object can be simplified by putting the ternary expression directly into the value for the optionLabel key.

Updated Code :

const renderNameLabel = (key: VKey, renderer: (searchField: SearchField) => VNodeChild) => {
const searchField = searchFields!.find(field => field.key === key)!
return renderer(searchField)
}

const customNameLabelRender = customNameLabel && isString(customNameLabel) ? slots[customNameLabel] : customNameLabel

const panelSlots = {
optionLabel: customNameLabelRender
? (option: SelectPanelData) => renderNameLabel(option.key, customNameLabelRender)
: undefined,
}

return (

Expand Down