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

fix(crop): [crop]modify crops component demo problem #2419

Merged
merged 2 commits into from
Oct 28, 2024
Merged

fix(crop): [crop]modify crops component demo problem #2419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d34ca8
fix(crops): [crops]modify crops component
James-9696 Oct 28, 2024
e8875fe
fix(crops): [crops]modify crops e2e-test
James-9696 Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions examples/sites/demos/pc/app/crop/min-crop-box-width-height-composition-api.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
:cropvisible="visible"
@update:cropvisible="visible = $event"
:src="imgUrl"
:min-crop-box-width="100"
:min-crop-box-height="100"
:min-container-height="200"
:min-container-width="800"
:min-crop-box-width="200"
:min-crop-box-height="350"
:min-container-height="300"
:min-container-width="650"
Comment on lines +8 to +11
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Crop box height constraint issue confirmed

The test file reveals that there's already a known bug with height constraints in the crop component. The test comment explicitly states: "此处有bug, 高度最小应该为 100， 实际最小为56.25" (translation: "There's a bug here, minimum height should be 100, but actual minimum is 56.25").

Given this context, setting min-crop-box-height (350px) larger than min-container-height (300px) is problematic because:

  1. The container height is enforced (test confirms container height is exactly 300px)
  2. The crop box cannot physically exceed its container's dimensions
  3. There's an existing height constraint bug in the implementation

Please adjust the constraints to ensure min-crop-box-height ≤ min-container-height:

-      :min-crop-box-height="350"
+      :min-crop-box-height="300"
🔗 Analysis chain

Verify crop box and container dimension constraints.

There appears to be a conflict in the minimum height constraints:

  • min-crop-box-height is set to 350px
  • min-container-height is set to 300px

This configuration might cause issues since the crop box cannot be larger than its container. Please verify if this is intentional or if the constraints need to be adjusted to ensure the crop box can fit within the container.

Consider one of these solutions:

  1. Decrease the min-crop-box-height to be less than or equal to min-container-height:
-      :min-crop-box-height="350"
+      :min-crop-box-height="300"
  1. Increase the min-container-height to accommodate the crop box:
-      :min-container-height="300"
+      :min-container-height="350"

Let's verify the component's behavior with these dimensions:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any existing tests or documentation that might provide insight into the expected dimension constraints
rg -g '*.{spec,test}.{js,ts,vue}' -l "min-crop-box|min-container" 

# Look for other examples or usage of the crop component to understand common patterns
rg -g '*.vue' -l "tiny-crop.*min-"

Length of output: 171

Script:

#!/bin/bash
# Let's check the test file content for any validation logic
cat examples/sites/demos/pc/app/crop/min-crop-box-width-height.spec.ts

# Let's also check for any documentation or implementation details in the crop component
rg -g '*.{md,vue}' "min-crop-box|min-container" -A 3 -B 3

# Search for the actual crop component implementation
ast-grep --pattern 'export default defineComponent({
  $$$
  props: {
    $$$
    minCropBoxHeight: $_,
    $$$
  }
  $$$
})'

Length of output: 3223

></tiny-crop>
</div>
</template>
Expand Down
8 changes: 4 additions & 4 deletions examples/sites/demos/pc/app/crop/min-crop-box-width-height.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ test('裁剪框最小宽高', async ({ page }) => {

box = await face.boundingBox()

expect(box.width).toBe(100)
expect(box.height).toBe(56.25) // 此处有bug, 高度最小应该为 100， 实际最小为56.25
expect(box.width).toBe(199.99996948242188)
expect(box.height).toBe(112.5) // 此处有bug, 高度最小应该为 100， 实际最小为56.25
Comment on lines +24 to +25
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

The test expectations are incorrect based on the component's configuration

The test expectations don't align with the actual component configuration:

  1. In the demo file min-crop-box-width-height.vue, the min-crop-box-height is set to 350 pixels.
  2. The test expects a minimum height of 100 pixels (as per comment), but gets 112.5 pixels.
  3. Both expectations are incorrect since they're below the configured minimum of 350 pixels.
  • Update the test expectations to match the component's configuration: 
    expect(box.width).toBe(350)  // Should match min-crop-box-width
expect(box.height).toBe(350) // Should match min-crop-box-height
🔗 Analysis chain

Review and fix the minimum height constraint.

  1. The height value (112.5) still doesn't meet the minimum requirement of 100px mentioned in the comment. This suggests the underlying bug hasn't been fully addressed.
  2. Consider simplifying the width expectation to 200 instead of 199.99996948242188 for better readability, unless this precise value is critical for the test.

Let's verify the minimum height constraint in the component:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for minCropBoxHeight property in the component
rg -A 5 "minCropBoxHeight|min-crop-box-height" --type vue

Length of output: 87

Script:

#!/bin/bash
# Search for minCropBoxHeight property in all files
rg -A 5 "minCropBoxHeight|min-crop-box-height"

# Also search for crop box height related configurations
rg -A 5 "cropBoxHeight|crop.*height"

# Look for the test file content to understand the context better
cat "examples/sites/demos/pc/app/crop/min-crop-box-width-height.spec.ts"

Length of output: 13030


// 判断container 大小为 800*200
box = await container.boundingBox()

expect(box.width).toBe(800)
expect(box.height).toBe(200)
expect(box.width).toBe(650)
expect(box.height).toBe(300)
})
8 changes: 4 additions & 4 deletions examples/sites/demos/pc/app/crop/min-crop-box-width-height.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
:cropvisible="visible"
@update:cropvisible="visible = $event"
:src="imgUrl"
:min-crop-box-width="100"
:min-crop-box-height="100"
:min-container-height="200"
:min-container-width="800"
:min-crop-box-width="200"
:min-crop-box-height="350"
:min-container-height="300"
:min-container-width="650"
Comment on lines +8 to +11
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Disproportionate dimensions in min-crop-box demo

The current dimensions (200x350) create an unusually tall and narrow crop box that doesn't align with common use cases, as evidenced by:

  • The basic usage demo which uses default proportions
  • The aspect-ratio demo which uses a more standard 7:5 ratio
  • Both the Vue and Composition API versions of the min-crop-box demo use these same unusual dimensions

Consider adjusting to more practical dimensions that better demonstrate the component's typical usage, such as:

  • Using a more balanced aspect ratio (e.g., 1:1 or 4:3)
  • Ensuring the container width provides adequate space for manipulation (current 650px is tight for a 200px min-width)
🔗 Analysis chain

Review the crop box and container dimensions

The current dimension settings raise a few concerns:

  1. The crop box dimensions (200x350) create a tall, narrow aspect ratio which might not represent a common use case
  2. The reduced container width (650px) combined with increased crop box dimensions (200x350) leaves limited space for user manipulation

Please confirm if these dimensions align with the intended use case. Consider:

  • Are these dimensions based on specific design requirements?
  • Would more balanced crop box dimensions (e.g., 200x200 or 350x350) better demonstrate the component's capabilities?
  • Is the container width sufficient given the increased crop box dimensions?
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other crop demos to compare dimension settings
rg -l "tiny-crop" examples/sites/demos/pc/app/crop/

Length of output: 2470

Script:

#!/bin/bash
# Let's check the content of min-crop-box-width-height.vue and min-crop-box-width-height-composition-api.vue
# to see if these dimensions are consistent across both implementations
cat examples/sites/demos/pc/app/crop/min-crop-box-width-height.vue
cat examples/sites/demos/pc/app/crop/min-crop-box-width-height-composition-api.vue

# Also check basic usage and aspect ratio demos for reference dimensions
cat examples/sites/demos/pc/app/crop/basic-usage.vue
cat examples/sites/demos/pc/app/crop/aspect-ratio.vue

Length of output: 2919

></tiny-crop>
</div>
</template>
Expand Down
1 change: 1 addition & 0 deletions packages/theme/src/crop/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@
margin-right: 8px;
padding: 4px;
cursor: pointer;
border-radius: 4px;

&:hover {
background: #f5f5f5;
Expand Down
Loading