feat(video-module): 为视频元素添加文本对齐属性 #429
Conversation
|
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/video-module/src/module/parse-elem-html.ts(5 hunks)
🔇 Additional comments (2)
packages/video-module/src/module/parse-elem-html.ts (2)
19-19: Consider alignment handling consistency across modules
The addition of text alignment specifically for video elements raises some architectural considerations:
- Does this align with how text alignment is handled in other editor modules?
- Should alignment be handled at a higher level in the editor's component hierarchy?
Consider reviewing the alignment implementation in other modules to ensure consistency across the editor.
Also applies to: 29-29, 40-40, 50-50, 70-70
✅ Verification successful
Based on the results, I can now generate the final response:
Text alignment implementation follows established patterns
The codebase shows consistent text alignment handling:
- Video module aligns with the basic-modules' justify functionality, both using the
textAlignproperty - The justify module (
basic-modules/src/modules/justify/style-to-html.ts) appears to be the core text alignment handler - Text alignment is consistently handled at the element level with proper HTML attribute management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar text alignment handling in other modules
rg -g "**/*.ts" "textAlign.*=.*'center'" -B 2 -A 2
# Search for other alignment-related code
ast-grep --pattern 'interface $_ {
$$$
textAlign: $_
$$$
}'
Length of output: 2769
19-19: Verify VideoElement interface update
The textAlign property has been added to the function signature and return object. Please ensure that the VideoElement interface in custom-types.ts has been updated to include this new property.
Also applies to: 29-29
✅ Verification successful
VideoElement interface already includes textAlign property
The VideoElement interface in packages/video-module/src/module/custom-types.ts has already been updated to include the textAlign property:
export type VideoElement = {
type: 'video'
key?: string
src: string
poster?: string
textAlign?: string // <-- Property exists
width?: string
height?: string
style?: videoStyle
children: EmptyText[]
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for VideoElement interface definition
ast-grep --pattern 'interface VideoElement {
$$$
}'
# Alternatively, search for the type definition file
rg -t typescript "interface VideoElement" -A 10
Length of output: 140
Script:
#!/bin/bash
# Search for interface or type definitions related to VideoElement
rg "VideoElement" --type ts -A 10
# Search for files that might contain type definitions
fd "types?.ts$" --type f
Length of output: 19542
dd9a06c to
b421815
Compare
- ��ㄨВ���瑙�棰����绱� HTML ��讹��浼����浜������榻���瑰�������峰����昏�� - 姝や慨��逛娇寰�瑙�棰����绱���互姝ｇ‘��扮户��跨�跺��绱���������榻���瑰�� - ��存�颁����稿�虫��璇���ㄤ��锛�澧����浜�瀵规����榻�灞���х��妫���� - wangeditor-next#428
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/video-module/src/module/parse-elem-html.ts (1)
50-51: Consider extracting text alignment logic to reduce duplicationWhile the implementation is correct, the text alignment extraction logic is duplicated between the iframe and video sections.
Consider extracting this logic into a helper function:
function getTextAlign($elem: JQuery): string { return styleStringToObject($elem.attr('style') || '')['text-align'] || 'center' } // Usage: textAlign = getTextAlign($elem)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/video-module/__tests__/parse-html.test.ts(2 hunks)packages/video-module/src/module/parse-elem-html.ts(5 hunks)
🔇 Additional comments (4)
packages/video-module/src/module/parse-elem-html.ts (2)
19-19: LGTM: Function signature and return object updates are consistent
The addition of the textAlign parameter with a default value of 'center' and its inclusion in the return object aligns well with the PR objectives.
Also applies to: 29-29
40-40: LGTM: Variable initialization is consistent
The initialization of textAlign with a default value of 'center' matches the default in genVideoElem.
packages/video-module/__tests__/parse-html.test.ts (2)
79-79: LGTM: Test case properly verifies text alignment
The test case correctly verifies that the text alignment is extracted and included in the parsed output.
103-103: LGTM: Test coverage is comprehensive
The video test case maintains consistency with the iframe test case and properly verifies the text alignment functionality.
|
测试了没问题，方便的话可以给 video module 加上对应的 align hoverbar ，照着 image module 相似的仿一下就行，或是我后面加上，哈哈😄 |
Changes Overview
Implementation Approach
Testing Done
Verification Steps
Additional Notes
Checklist
Related Issues
Summary by CodeRabbit
New Features
textAlignparameter for video element generation, allowing customization of text alignment.Bug Fixes
<iframe>and<video>elements to ensure proper text alignment is applied.