examples/sites/demos/pc/app/drawer/placement-composition-api.vue (3)

8-15: LGTM! Consider adding an aria-label for accessibility.

The implementation of the drawer for 'left' and 'right' placements looks good. The conditional rendering and prop bindings are correct.

Consider adding an aria-label to improve accessibility:

 <tiny-drawer
   v-if="placement === 'left' || placement === 'right'"
   title="标题"
   :placement="placement"
   v-model:visible="visible"
+  :aria-label="`${placement} drawer`"
 >
   <div style="padding: 32px">left或者right内容区域</div>
 </tiny-drawer>

---

17-24: LGTM! Consider making the height dynamic and adding an aria-label.

The implementation of the drawer for 'top' and 'bottom' placements looks good. The conditional rendering and prop bindings are correct.

Consider the following improvements:

1. Make the height prop dynamic to allow for more flexibility:

-  height="400px"
+  :height="drawerHeight"

Then add a drawerHeight ref in the <script setup> section:

const drawerHeight = ref('400px')

2. Add an aria-label to improve accessibility:

 <tiny-drawer
   v-if="placement === 'top' || placement === 'bottom'"
   title="标题"
   :placement="placement"
   v-model:visible="visible"
   :height="drawerHeight"
+  :aria-label="`${placement} drawer`"
 >
   <div style="padding: 32px">top或者bottom区域</div>
 </tiny-drawer>

These changes will make the component more flexible and accessible.

---

Issues Found

The review highlights several valid concerns regarding the placement-composition-api.vue component:

1. Missing Tests:

   • No test files are associated with placement-composition-api.vue. It's essential to add comprehensive tests to ensure the new functionality works as intended.

2. Height Prop for Left/Right Placements:

   • The height prop is currently only applied to top/bottom placements. Consider extending the height prop to left/right placements for consistency and enhanced functionality.

3. Documentation Updates:

   • Documentation does not mention the height prop for the drawer component. Update the relevant documentation to reflect this addition and provide clear guidance on its usage.

To address these issues, please take the following actions:

• Add Tests:

  • Create unit tests for placement-composition-api.vue to cover various placement scenarios and ensure the height prop functions correctly.

• Extend Height Prop Usage:

  • Modify the component to include the height prop for left/right placements, ensuring consistent behavior across all placements.

• Update Documentation:

  • Enhance the component's documentation to include explanations and examples of the height prop usage for left/right placements.

🔗 Analysis chain

Line range hint 1-41: Overall structure is good, but consider adding documentation and tests.

The implementation demonstrates the drawer component with different placements effectively. However, there are a few points to consider:

1. Add comments or documentation to explain the component's purpose and usage.
2. The PR objectives mentioned adding a height prop, which is implemented for top/bottom placements. Consider if it should also be applicable for left/right placements.
3. As noted in the PR objectives, tests and documentation updates are missing. These should be added to ensure the new functionality is properly tested and documented.

To address these points:

1. Add a comment at the top of the file explaining the component's purpose and usage.
2. If applicable, add a height prop to the left/right drawer as well.
3. Create unit tests for the new functionality.
4. Update the component's documentation to reflect the new height prop.

To check for existing tests and documentation, run:

---

Verification Successful: computedHeight Function Implemented Correctly

The computedHeight function has been successfully located and implemented as expected. The addition aligns with the PR objectives and maintains consistency within the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of computedHeight function

# Test: Search for the computedHeight function implementation
rg --type typescript -A 10 'export const computedHeight'

Length of output: 279

---

Script:

#!/bin/bash
# Description: Search for the computedHeight function implementation in TypeScript files without using --type flag

# Find all .ts and .tsx files and search for 'export const computedHeight'
fd --extension ts --extension tsx --exec rg -A 10 'export const computedHeight' {}

Length of output: 345

packages/renderless/src/drawer/index.ts (2)

20-32: LGTM with suggestions for improvement

The computedHeight function is well-implemented and consistent with the existing computedWidth function. However, consider the following suggestions to improve robustness and type safety:

1. Add support for different units (e.g., percentages, em) in the height property.
2. Implement validation to ensure the height is a positive number.
3. Instead of using type assertion as IDrawerCT, consider updating the type definition of constants to avoid potential type issues.

Here's a potential refactoring that addresses these points:

import { isNumber } from '../common/deps/utils' // Assume this utility function exists

export const computedHeight = ({
  state,
  designConfig,
  props,
  constants
}: Pick<IDrawerRenderlessParams, 'state' | 'designConfig' | 'props' | 'constants'>) =>
  (): string => {
    if (state.height) {
      return isNumber(state.height) ? `${state.height}px` : state.height
    }
    
    const height = props.height || designConfig?.constants?.DEFAULT_HEIGHT || constants.DEFAULT_HEIGHT
    
    if (isNumber(height)) {
      return height > 0 ? `${height}px` : '0px'
    }
    
    return height || '0px'
  }

This refactored version:

1. Supports different units by checking if the height is a number before appending 'px'.
2. Ensures the height is non-negative when it's a number.
3. Removes the need for type assertion by assuming constants.DEFAULT_HEIGHT is correctly typed.

To ensure that the IDrawerCT interface includes the DEFAULT_HEIGHT property, run the following script:

#!/bin/bash
# Description: Verify the IDrawerCT interface includes DEFAULT_HEIGHT

# Test: Search for the IDrawerCT interface definition
rg --type typescript 'interface IDrawerCT' -A 10

This will help us confirm that the DEFAULT_HEIGHT property is properly defined in the IDrawerCT interface, potentially eliminating the need for the type assertion.

---

20-32: Verify integration and update documentation for the new height property

The addition of the computedHeight function is a good enhancement to the drawer component. To ensure proper integration and maintainability, please address the following points:

1. Update the drawer component to use this new computedHeight function.
2. Add documentation for the new height property, including its usage and any constraints.
3. Update existing tests and add new ones to cover the height functionality.

To help verify the integration, run the following script to check for updates in related files:

This script will help us verify that the necessary updates have been made across the codebase to support the new height property.

examples/sites/demos/apis/drawer.js (1)

Line range hint 1-186: Summary: New 'height' property added to drawer component API

The changes in this file are limited to the addition of the 'height' property for the drawer component. This new property enhances the component's flexibility by allowing users to set a custom height for the drawer, particularly useful for top and bottom placements in PC mode.

Key points:

1. The new property is well-documented and consistent with existing properties.
2. It's correctly limited to 'pc' mode.
3. The stability version is properly noted.

Overall, this is a valuable addition to the drawer component that shouldn't introduce any breaking changes to existing implementations.

To ensure a smooth integration of this new feature, please:

1. Verify the drawer component's internal logic handles the new 'height' property correctly.
2. Update the 'placement' demo for PC to showcase the usage of the 'height' property.
3. Consider adding validation for the 'height' property value in the component's props definition.