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(comp:cascader): add IxCascaderPanel component #1481

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?

抽离级联选择面板 IxCascaderPanel

image

Other information

@@ -200,7 +199,7 @@ describe('Cascader', () => {
expect(getAllOptionGroup(wrapper).length).toBe(2)

await wrapper
.findComponent(OverlayContent)
.findComponent(Panel)
.findAll('.ix-cascader-option-group')[1]
.find('.ix-cascader-option')
.trigger('click')
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:

The code patch above includes modifications to the imports, functions, and tests for a Cascader component. The changes include importing Panel instead of OverlayContent, changing the getAllOptionGroup function to use Panel instead of OverlayContent, and modifying a test to use Panel instead of OverlayContent.

Overall, the code looks good and there do not appear to be any bug risks or issues with the modifications. However, it would be beneficial to add more comments explaining the purpose and details of the modifications, as well as why they were made. This will help other developers better understand the code. Additionally, if possible, it would also be beneficial to add unit tests to ensure the changes work as expected.


## en

use `IxCascaderPanel` only.
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 is written in YAML language and it is probably used for a documentation. It has the following components: order, title (with zh and en version), zh and en documentations.

From a code review perspective, it seems there are no bugs or risks. However, it could be improved by adding more details to the description of the components and documentations to make them more clear. Also, consider using other languages if needed to better explain the components and documentations.

@idux-bot
Copy link

idux-bot bot commented Mar 2, 2023

This preview will be available after the AzureCI is passed.

]
</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.

the code review:

  1. The code looks valid and complete, and there is no obvious bug risk.
  2. It might be better to consider using a more descriptive name for the 'onSelect' function instead of just 'onSelect'.
  3. If 'dataSource' is going to be used in multiple components, it could be stored in a separate file and imported into each component that needs it. This would make the code easier to maintain.
  4. The code could use some minor formatting changes to make it easier to read, such as indenting the code and using consistent spacing.

| 名称 | 说明 | 参数类型 | 备注 |
| --- | --- | --- | --- |
| `empty` | 自定义空状态 | - | - |
| `optionLabel` | 自定义选项的文本 | `data: SelectOption` | - |
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.

The code looks generally well-structured and easy to read. The variable and function names are descriptive, making it easier to understand what each component does. However, there are a few areas where the code could be improved:

  1. The indentation of the code could be improved to make it more readable.
  2. Consider adding comments to explain any complex logic or abstract concepts.
  3. Make sure all variables are properly typed and checked for errors.
  4. Avoid using unnecessary global variables or constants.
  5. Make sure that all functions and classes are properly documented.
  6. Verify that all code is optimized for performance.
  7. Make sure that all data structures are properly validated before use.
  8. Make sure that all user input is properly sanitized.
  9. Verify that all functions and classes are thoroughly unit tested.


export type {
CascaderInstance,
CascaderComponent,
CascaderPublicProps as CascaderProps,
CascaderPanelInstance,
CascaderPanelComponent,
CascaderPanelPublicProps as CascaderPanelProps,
CascaderData,
CascaderExpandTrigger,
CascaderSearchFn,
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-structured and correctly indented.
  2. The types of variables are properly declared.
  3. The imports and exports are correctly formatted.
  4. The code is not optimized for performance, as there are redundant lines of code that can be removed.
  5. There are no obvious bugs present in the code.
  6. The code could be improved by optimizing it for better performance, as mentioned above.

props: CascaderProps,
mergedDataMap: ComputedRef<Map<VKey, MergedData>>,
): ActiveStateContext {
export function useActiveState(mergedDataMap: ComputedRef<Map<VKey, MergedData>>): ActiveStateContext {
const [activeKey, setActiveKey] = useState<VKey | undefined>(undefined)

const activeData = computed(() => {
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. This code patch removes the CascaderProps argument from the useActiveState function and instead uses the mergedDataMap argument. The activeKey and setActiveKey variables are defined using the useState utility, and the activeData variable is computed using the mergedDataMap argument.

In terms of bugs and improvements, this code patch looks fine. There are no obvious bugs and no improvements that need to be made.

@@ -56,7 +56,7 @@ export function useDataSource(
}

export function convertMergedData(
props: CascaderProps,
props: CascaderProps | CascaderPanelProps,
getKey: GetKeyFn,
childrenKey: string,
labelKey: string,
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 patch changes the imported types of CascaderProps to include CascaderPanelProps. This allows the useDataSource function to accept a CascaderPanelProps object instead of just a CascaderProps object, allowing for more flexibility in how cascader data is sourced.
  2. The convertMergedData function has also been changed to accept a CascaderPanelProps object, allowing for the same flexibility as before.
  3. There do not appear to be any risks or bugs in this code patch. It does appear to be an improvement over the previous version, as it adds more flexibility in sourcing cascader data.

@@ -24,7 +24,7 @@ export interface ExpandableContext {
}

export function useExpandable(
props: CascaderProps,
props: CascaderProps | CascaderPanelProps,
mergedGetKey: ComputedRef<GetKeyFn>,
mergedGetDisabled: ComputedRef<GetDisabledFn>,
mergedChildrenKey: ComputedRef<string>,
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 adds a new type CascaderPanelProps to the import statement and uses it instead of CascaderProps in the useExpandable function. This is done to allow for more flexibility when using the Cascader component.
  2. The code looks good and there don't seem to be any bugs or risks associated with the code.
  3. Possible improvement suggestions include making sure the CascaderPanelProps is properly documented and that it is used in all relevant places where the Cascader component is used.

})
return [...indeterminateKeySet]
})
}
Copy link

Choose a reason for hiding this comment

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

:

  1. Overall, the code seems well-structured and is easy to follow.
  2. I suggest to add some comments at the beginning of the function to explain what the function is doing.
  3. I suggest to use more descriptive variable names. For example, instead of 'currData' it could be 'currentData'.
  4. The use of Set ensures that the returned array contains no duplicate values.
  5. The while loop should have a condition to check whether the loop should continue, otherwise it might lead to infinite loop.
  6. I suggest to add a type check for the parameters passed into the function to make sure the function will not fail when passing wrong types of data.

'onUpdate:loadedKeys': setLoadedKeys,
onSelect,
}))
}
Copy link

Choose a reason for hiding this comment

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

!

Code Review:

This code looks relatively good. The usePanelProps function is well commented and easy to understand. It looks like the useControlledProp function is being used correctly and safely. I would suggest adding more comments to explain the purpose of the function, as well as adding more explicit types for the parameters and return values. Additionally, the function could be refactored to reduce the amount of duplicate properties in the computed object.

@@ -51,7 +50,7 @@ export function useSearchable(
return { searchedData }
}

function useSearchFn(props: CascaderProps, mergedLabelKey: ComputedRef<string>) {
function useSearchFn(props: CascaderPanelProps, mergedLabelKey: ComputedRef<string>) {
return computed(() => {
const searchFn = props.searchFn
if (isFunction(searchFn)) {
Copy link

Choose a reason for hiding this comment

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

from the beginning:

  1. The import statements have been changed from '@idux/components/types' to '../types', this change has been made to improve the readability of the code and make it easier to identify the imports being used in the code.
  2. The 'props' parameter has been changed from 'CascaderProps' to 'CascaderPanelProps'. This is likely to ensure that only the props related to the Cascader Panel are being used in the code.
  3. The 'inputValue' parameter has been removed entirely, and the 'props.searchValue' is now being used instead. This is likely to reduce the complexity of the code and improve performance by not having to use an extra computed ref.
  4. The 'useSearchFn' function has been updated to use 'CascaderPanelProps' instead of 'CascaderProps'. This is likely to ensure that only the necessary props are being used in the function.

Overall, this looks like a good patch of code with no major bugs or risks. The changes made seem to be mostly for readability and performance improvements.

const dataMap = mergedDataMap.value
return selectedKeys.value.map(key => dataMap.get(key)!).filter(Boolean)
})
}
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

  1. The code is well commented and has a license header at the top which is good.

  2. The code imports the necessary packages and types and also uses TypeScript to ensure type safety.

  3. The useSelectedData function is well structured, it takes in two arguments, a ComputedRef of selectedKeys and a ComputedRef of mergedDataMap.

  4. It uses the ComputedRef to return an array of MergedData and label.

  5. The code is short, concise and easy to read.

Overall the code looks good, however there are some things that can be improved:

  1. The function should have more descriptive names and be broken into smaller functions if needed.

  2. The code should use more descriptive variable names and follow the coding style guidelines.

  3. The code should be tested to make sure it works correctly.

  4. The code should be optimized for performance.

selectedLimit,
selectedLimitTitle,
}
}
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 good practice to include comments in your code. This code includes some comments which are helpful in understanding the purpose and behavior of the code.

  2. The code uses imports from various libraries such as '@idux/cdk/utils' and 'vue'. This ensures that you are using the latest versions of these libraries and reduces the risk of errors.

  3. The code has an interface called SelectedLimitContext which provides a structure for the data being used in the code. This makes it easier to understand the code and makes it easier to debug in case of errors.

  4. The useSelectedLimit() function uses computed properties to derive the selectedLimit and selectedLimitTitle variables. This is a good practice as it ensures that the values are always up to date and any changes in the underlying data are reflected in the output.

  5. The code also uses the useGlobalConfig() function from the '@idux/components/config' library which is useful for retrieving global configuration settings. This helps to keep the code DRY (Don't Repeat Yourself) and maintainable.

Overall, the code looks well-structured and well-written with minimal risks of bugs or errors. However, it would be better to add more comments in order to make the code more self-explanatory.

return <div>{overlayRender ? overlayRender(children) : children}</div>
}
},
})
Copy link

Choose a reason for hiding this comment

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

our code review.

The code patch looks good and is easy to read. The indentation is consistent and there are no syntax errors. There are a few best practices that can be followed:

  1. Add comments to explain the code.
  2. Use more descriptive variable names.
  3. Break down long functions into smaller, easier to manage functions.
  4. Check for any potential bugs by using unit tests.
  5. Make sure that all of the code is properly documented.
  6. Follow standard coding conventions.
  7. Use a linter to ensure that the code follows a consistent style.

const customAdditional = cascaderProps.customAdditional
? cascaderProps.customAdditional({ data: rawData, index: props.index })
const customAdditional = cascaderPanelProps.customAdditional
? cascaderPanelProps.customAdditional({ data: rawData, index: props.index })
: undefined

return (
Copy link

Choose a reason for hiding this comment

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

with a quick review:

The code looks good and should accomplish the task; however, there are a few minor improvements that can be made.

  1. The import of callEmit from @idux/cdk/utils is unnecessary since it is already imported from @idux/components/utils.
  2. The cascaderToken has been changed to cascaderPanelToken but this has not been done in all places. All references to cascaderToken should be changed to cascaderPanelToken.
  3. The _handleSelect function can be simplified by removing the call to setOverlayOpened.
  4. The handleExpand function can be simplified by passing it the key as an argument.
  5. The searchValue variable is used twice and should be declared only once.
  6. The customAdditional function can be simplified by passing in the index of the props.
  7. Finally, the code should be formatted for readability.

itemRender={itemRender}
virtual={virtual}
/>
)
} else {
children = <ɵEmpty v-slots={slots} empty={cascaderProps.empty} />
children = <ɵEmpty v-slots={slots} empty={cascaderPanelProps.empty} />
}
Copy link

Choose a reason for hiding this comment

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

The code looks good, there is no bug risk. Some improvement suggestions are:

  1. Use const instead of let when declaring variables.
  2. Declare variables as close to their usage as possible.
  3. Add comments to explain the code logic.
  4. Use meaningful variable names.
  5. Use proper indentation and whitespace for better readability.


export const CASCADER_PANEL_DATA_TOKEN: InjectionKey<DataSourceContext & SelectedStateContext> =
Symbol('CASCADER_PANEL_DATA_TOKEN')
export const cascaderPanelToken: InjectionKey<CascaderPanelContext> = Symbol('cascaderPanelToken')
export const cascaderToken: InjectionKey<CascaderContext> = Symbol('cascaderToken')
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 patch adds a new import type 'SelectedLimitContext' from './composables/useSelectedLimit'. This is correct and necessary for the code to work properly.

  2. It also adds an import type 'CascaderPanelProps, CascaderProps' from './types'. This is also correct and necessary for the code to work properly.

  3. It imports VKey from '@idux/cdk/utils' which is necessary for the code to work properly.

  4. It adds an interface 'CascaderPanelContext' which is necessary for the code to work properly.

  5. It adds two constants 'CASCADER_PANEL_DATA_TOKEN' and 'cascaderPanelToken' which are both necessary for the code to work properly.

  6. There does not appear to be any bug risks or areas for improvement in this patch.

flex-wrap: nowrap;
align-items: flex-start;
font-size: @cascader-option-font-size;
overflow: auto;
}
}
Copy link

Choose a reason for hiding this comment

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

The code patch looks good overall. However, there may be a few risks and improvements that could be made:

Risks:

  • There is no closing bracket for the &-search-wrapper class
  • The .reset-component() function may not be defined correctly

Improvements:

  • Use more descriptive variable and class names
  • Break up the code into smaller sections to make it easier to read
  • Add comments to explain the purpose of each section of code

@@ -94,6 +94,7 @@ const components = [
IxCardGrid,
IxCarousel,
IxCascader,
IxCascaderPanel,
IxComment,
IxCheckbox,
IxCheckboxGroup,
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. This code patch seems to be importing two components from @idux/components/cascader, IxCascader and IxCascaderPanel. This is a good addition since adding additional components should add more functionality and usability to the application.

  2. The code also adds IxCascaderPanel to the components array. This allows users to access the new component via the components array.

  3. There does not appear to be any potential bugs or risks in this patch. All the added code looks valid and complete.

  4. As an improvement, it might be good to add some documentation alongside this change so other developers can easily understand what changes have been made and why. This can help ensure that future changes are made correctly.

@@ -103,6 +103,7 @@ declare module 'vue' {
IxCardGrid: CardGridComponent
IxCarousel: CarouselComponent
IxCascader: CascaderComponent
IxCascaderPanel: CascaderPanelComponent
IxCheckbox: CheckboxComponent
IxCheckboxGroup: CheckboxGroupComponent
IxCol: ColComponent
Copy link

Choose a reason for hiding this comment

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

  1. Check for typos and errors:
    No typos and errors were noticed in the code.

  2. Review the logic of the code snippet:
    The code snippet adds two imports, one for CascaderComponent and one for CascaderPanelComponent. This is likely to add functionality to the application which would allow the user to create cascading menus.

  3. Ensure that the code follows coding conventions and best practices:
    The code follows the usual JavaScript syntax and conventions. It also imports the components from the '@idux/components/cascader' module, which is a good practice.

@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #1481 (35b6f3f) into main (f45776d) will decrease coverage by 0.06%.
The diff coverage is 83.67%.

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

@@            Coverage Diff             @@
##             main    #1481      +/-   ##
==========================================
- Coverage   92.80%   92.74%   -0.06%     
==========================================
  Files         331      331              
  Lines       30661    30742      +81     
  Branches     3527     3533       +6     
==========================================
+ Hits        28454    28511      +57     
- Misses       2207     2231      +24     
Impacted Files Coverage Δ
packages/components/cascader/src/Cascader.tsx 84.68% <63.63%> (-11.39%) ⬇️
packages/components/cascader/src/token.ts 100.00% <100.00%> (ø)
packages/components/cascader/src/types.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.

@danranVm danranVm merged commit e044390 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