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(pro:search): name select label not rendering #1493

Merged
merged 1 commit into from Mar 7, 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?

name 下拉框不渲染

What is the new behavior?

修复以上问题

Other information

@idux-bot
Copy link

idux-bot bot commented Mar 6, 2023

This preview will be available after the AzureCI is passed.

@@ -48,7 +48,7 @@ export function createNameSegment(

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

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.

@sallerli1 sallerli1 force-pushed the fix-pro-search-name-label-slot branch from 460cb74 to f7976df Compare March 6, 2023 13:04

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

@@ -48,7 +48,7 @@ export function createNameSegment(

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

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.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #1493 (91ecba7) into main (1be87e8) will not change coverage.
The diff coverage is n/a.

❗ Current head 91ecba7 differs from pull request most recent head 9b18407. Consider uploading reports for the commit 9b18407 to get more accurate results

@@           Coverage Diff           @@
##             main    #1493   +/-   ##
=======================================
  Coverage   92.73%   92.73%           
=======================================
  Files         331      331           
  Lines       30752    30752           
  Branches     3533     3533           
=======================================
  Hits        28519    28519           
  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.

@@ -48,7 +48,7 @@ export function createNameSegment(

const panelSlots = {
optionLabel: isString(_customNameLabel)
? (option: SelectPanelData) => renderNameLabel(option.key, slots[_customNameLabel])
? slots[_customNameLabel] && ((option: SelectPanelData) => renderNameLabel(option.key, slots[_customNameLabel]))
Copy link
Member

Choose a reason for hiding this comment

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

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

其他类似地方都改一下。

@sallerli1 sallerli1 force-pushed the fix-pro-search-name-label-slot branch from f7976df to 9b18407 Compare March 7, 2023 01:41

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

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

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 (

@danranVm danranVm merged commit cb4b86a into IDuxFE:main Mar 7, 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