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: elem id add type #296

Merged
merged 13 commits into from
Oct 29, 2024
Merged

Conversation

cycleccc
Copy link
Collaborator

@cycleccc cycleccc commented Oct 28, 2024

Changes Overview

Implementation Approach

Testing Done

Verification Steps

Additional Notes

Checklist

  • I have created a changeset for this PR if necessary.
  • My changes do not break the library.
  • I have added tests where applicable.
  • I have followed the project guidelines.
  • I have fixed any lint issues.

Related Issues

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced an Element ID type in the Wangeditor Next editor for enhanced element identification.
    • Added a new utility function isDOMText to improve text node handling within the editor.
    • New type declaration ElementType for better type safety.
  • Improvements

    • Enhanced logic for generating DOM node IDs to include element types.
    • Improved readability and organization of code across multiple files, including better handling of conditional statements.
  • Development Tools

    • Added a new script "dev:sh" for streamlined development builds.
    • Introduced a new script build-base.sh for automating the packaging of multiple software packages.

Copy link

changeset-bot bot commented Oct 28, 2024

⚠️ No Changeset found

Latest commit: 049309b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Oct 28, 2024

Warning

Rate limit exceeded

@cycleccc has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 6 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Files that changed from the base of the PR and between 425aac8 and 2484c3f.

Walkthrough

The changes introduce a new feature that adds an Element ID type to the Wangeditor Next editor and core packages, enhancing element identification functionality. Modifications include updates to test cases, import reorganization, and improvements in code readability across various files. Key functions such as genElemId and renderElement now incorporate the element type in ID generation. Additionally, a new utility function for identifying DOM text nodes is introduced, and a new script for building packages is added to the project.

Changes

File Change Summary
.changeset/loud-dogs-fly.md Added Element ID type to Wangeditor Next editor and core packages.
packages/core/tests/editor/dom-editor.test.ts Updated test for toDOMNode to include p.type in ID generation and modified variable type casting.
packages/core/src/editor/plugins/with-content.ts Reorganized imports, improved readability in several functions, and refined ID generation logic to include node.type.
packages/core/src/render/element/renderElement.tsx Adjusted ID generation in renderElement to include elemNode.type and improved readability with block syntax.
packages/core/src/render/helper.ts Updated genElemId function to accept an additional type parameter.
packages/core/src/editor/plugins/with-max-length.ts Reformatted code for readability without altering functionality.
packages/core/src/parse-html/parse-common-elem-html.ts Rearranged imports, simplified text appending logic, and improved checks for DOM elements.
packages/core/src/parse-html/parse-elem-html.ts Replaced NodeType import with isDOMText for checking text nodes.
packages/core/src/utils/dom.ts Introduced NodeType enum for DOM node types and refactored functions to use this enum.
package.json Added new script "dev:sh" to execute a shell script for development builds.
packages/core/src/editor/dom-editor.ts Introduced isDOMText utility function for improved text node identification.
packages/custom-types.d.ts Added new type declaration ElementType for better type safety.
scripts/build-base.sh New script to automate the packaging of multiple software packages.

Possibly related PRs

🐰 In the code where elements play,
A new ID type leads the way.
With clearer paths and types defined,
Our editor's strength is now refined.
Hop along, let’s build and cheer,
For every change brings us near! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (4)
.changeset/loud-dogs-fly.md (1)

6-6: Enhance the commit message clarity.

While the message follows conventional commit format with "feat:", it could be more descriptive about what the change accomplishes.

Consider updating to something like:

-feat: elem id add type
+feat: add type parameter to element ID generation
packages/core/src/render/helper.ts (1)

6-7: Add JSDoc documentation for the new parameter.

The function's documentation should be updated to describe the new type parameter and provide examples of valid values.

 /**
  * @description formats helper
  * @author wangfupeng
+ * @param type The type of element (e.g., 'paragraph', 'header')
+ * @param id Unique identifier for the element
+ * @returns Formatted element ID string
  */
packages/core/src/render/element/renderElement.tsx (1)

50-53: LGTM! Code style improvements enhance readability.

Good improvements:

  • Using const for renderElem improves code safety
  • Block syntax for conditionals enhances readability

Consider adding a type annotation for renderElem to improve code clarity:

- const renderElem = getRenderElem(type)
+ const renderElem: RenderElementFunction = getRenderElem(type)

Also applies to: 107-121

packages/core/src/editor/plugins/with-content.ts (1)

Line range hint 1-452: Consider implementing a more robust error handling strategy.

While the changes successfully implement element ID type support, consider enhancing the error handling strategy across the editor operations. Some suggestions:

  1. Implement a centralized error handling mechanism for editor operations
  2. Add error boundaries for critical operations like HTML parsing and content transformation
  3. Consider adding logging for debugging purposes

This would improve the maintainability and reliability of the editor.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 39ed840 and 30b9099.

📒 Files selected for processing (5)
  • .changeset/loud-dogs-fly.md (1 hunks)
  • packages/core/tests/editor/dom-editor.test.ts (2 hunks)
  • packages/core/src/editor/plugins/with-content.ts (15 hunks)
  • packages/core/src/render/element/renderElement.tsx (5 hunks)
  • packages/core/src/render/helper.ts (1 hunks)
🔇 Additional comments (8)
packages/core/src/render/helper.ts (1)

6-7: Verify all callers of genElemId are updated.

This is a breaking change as the function now requires a type parameter. We need to ensure all existing calls are updated.

✅ Verification successful

No issues found - function usage is consistent

The verification shows that:

  • The genElemId in update-view.ts is a different function with its own implementation for textarea elements
  • The imported genElemId from helper.ts is correctly used with both type and id parameters in:
    • renderElement.tsx: genElemId(elemNode.type, key.id)
    • with-content.ts: genElemId(node.type, key.id)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing calls to genElemId to verify they've been updated with the type parameter
rg "genElemId\(" --type ts

Length of output: 474


Script:

#!/bin/bash
# Get more context around the genElemId function in update-view.ts to understand if it's a different function
rg "genElemId" packages/core/src/text-area/update-view.ts -B 2 -A 2

# Get more context around the usage in renderElement.tsx and with-content.ts
rg "genElemId" packages/core/src/render/element/renderElement.tsx -B 2 -A 2
rg "genElemId" packages/core/src/editor/plugins/with-content.ts -B 2 -A 2

Length of output: 1300

packages/core/src/render/element/renderElement.tsx (2)

6-22: LGTM! Import organization improvements.

The consolidation and reordering of imports enhance code organization and readability.


40-40: Verify element type availability and usage consistency.

The addition of elemNode.type to genElemId aligns with the PR objective. However, we should verify:

  1. The type parameter is always available
  2. All other callers of genElemId have been updated
✅ Verification successful

Based on the shell script results, I can now generate the final response:

genElemId usage is consistent with type parameter

The verification shows that:

  1. The genElemId function in packages/core/src/render/helper.ts is correctly defined to accept both type and id parameters
  2. All usages of genElemId in the codebase pass both parameters:
    • In renderElement.tsx: genElemId(elemNode.type, key.id)
    • In with-content.ts: genElemId(node.type, key.id)

The element type is consistently available through the type property of element nodes, as evidenced by numerous usages throughout the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all genElemId calls to ensure they've been updated with type parameter
rg "genElemId\(" --type ts --type tsx

# Search for potential element type definitions to verify type availability
ast-grep --pattern "interface Element {
  type: $_
  $$$
}"

Length of output: 133


Script:

#!/bin/bash
# Search for all genElemId calls with ripgrep, fixing the file type flags
rg "genElemId\(" -t typescript -t tsx

# Search for element type definitions using a more specific pattern
ast-grep --pattern 'interface $_ {
  type: $_
}'

# Also search for type in element node references
ast-grep --pattern '$_.type'

Length of output: 8398


Script:

#!/bin/bash
# Search for genElemId function definition and usages with plain grep
rg "genElemId" -l

# Then use cat to see the content of files containing genElemId
rg "genElemId" -l | xargs cat

# Search for Element interface or type definition
ast-grep --pattern 'type Element = {
  type: $_
  $$$
}'

Length of output: 18695

packages/core/__tests__/editor/dom-editor.test.ts (2)

8-8: LGTM: Import statement aligns with type requirements.

The addition of CustomElement type import supports the enhanced element identification feature.


152-159: LGTM: Test updates properly verify the new element ID format.

The changes correctly test the new element identification system by:

  1. Properly type-casting the element
  2. Verifying the updated ID format that includes the element type

Let's verify the consistency of this ID format across the codebase:

✅ Verification successful

Let me gather more information about the toDOMNode implementation since the previous pattern didn't match exactly.


Let me search specifically for the DomEditor implementation since that's what we're verifying.


Let me try one final search to locate the DomEditor implementation and element ID generation.


Based on the evidence gathered, I can now provide the final response:

✓ Element ID format is consistently implemented across the codebase

The test correctly verifies the element ID format which is defined in packages/core/src/render/helper.ts. The genElemId helper function uses the exact same format w-e-element-${type}-${id} that's being tested, confirming that the test is properly aligned with the implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent element ID generation pattern
# Expected format: w-e-element-${type}-${id}

# Search for element ID generation patterns
echo "Checking ID generation patterns:"
rg -n "w-e-element-" --type ts

# Search for toDOMNode implementations
echo -e "\nChecking toDOMNode implementations:"
ast-grep --pattern 'toDOMNode($$$) { $$$}'

Length of output: 428


Script:

#!/bin/bash
# Search for toDOMNode with class/object context
ast-grep --pattern 'class $_ {
  $$$
  toDOMNode($$$) {
    $$$
  }
  $$$
}'

# Also check for any DOM node ID related code
rg -n "createElement|getElementById" --type ts -A 3

Length of output: 15845


Script:

#!/bin/bash
# Search for DomEditor class implementation
ast-grep --pattern 'class DomEditor {
  $$$
}'

# Also search for element ID generation related code
rg -n "getElementId|generateId" --type ts -A 3

Length of output: 97


Script:

#!/bin/bash
# Search for files containing DomEditor
rg -l "DomEditor" --type ts

# Search for element ID generation in render/helper.ts which we found earlier
cat packages/core/src/render/helper.ts

Length of output: 11770

packages/core/src/editor/plugins/with-content.ts (3)

6-21: LGTM! Import statements are well organized.

The imports are logically grouped and the addition of genElemId aligns with the PR's objective of adding element ID type support.


35-35: LGTM! Improved code clarity with explicit braces.

The addition of braces to the if statement follows best practices and prevents potential bugs from implicit statement blocks.


430-430: LGTM! Improved code formatting.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (6)
packages/core/src/parse-html/parse-elem-html.ts (1)

Line range hint 65-65: Consider replacing nodeType comparison with isDOMText.

For consistency with the earlier changes, consider replacing the direct nodeType !== 3 comparison with !isDOMText().

-    if ($elem[0].childNodes.length > 0 && $elem[0].childNodes[0].nodeType !== 3) {
+    if ($elem[0].childNodes.length > 0 && !isDOMText($elem[0].childNodes[0])) {
packages/core/src/editor/plugins/with-max-length.ts (1)

Line range hint 74-136: Consider enhancing error handling and user feedback.

While the maxLength functionality works, it silently fails when limits are exceeded. Consider adding error events or callbacks to notify users when content is truncated or rejected.

Example implementation:

interface MaxLengthOptions {
  onLimitExceeded?: (remaining: number) => void;
}

// Add to config and emit events when content is truncated
if (leftLength < text.length) {
  editor.config.onLimitExceeded?.(leftLength);
  // ... existing logic
}
packages/core/src/parse-html/parse-common-elem-html.ts (1)

Line range hint 164-174: Consider extracting nested conditions into a separate function.

The nested conditions for handling void elements and children could be more readable if extracted into a separate function.

Consider refactoring like this:

- if (!isVoid) {
-   // 非 void ,如果没有 children ,则取纯文本
-   if (children.length === 0) {
-     elem.children = [{ text: $elem.text().replace(/\s+/gm, ' ') }]
-   }
-
-   // 处理 style
-   PARSE_STYLE_HTML_FN_LIST.forEach(fn => {
-     elem = fn($elem[0], elem, editor) as Element
-   })
- }
+ function processNonVoidElement(elem: Element, $elem: Dom7Array, children: Descendant[], editor: IDomEditor): Element {
+   if (children.length === 0) {
+     elem.children = [{ text: $elem.text().replace(/\s+/gm, ' ') }]
+   }
+   
+   return PARSE_STYLE_HTML_FN_LIST.reduce((acc, fn) => 
+     fn($elem[0], acc, editor) as Element, elem)
+ }
+
+ if (!isVoid) {
+   elem = processNonVoidElement(elem, $elem, children, editor)
+ }
packages/core/src/utils/dom.ts (3)

87-96: Well-structured enum for DOM node types!

Good practice replacing magic numbers with named constants. This improves code readability and maintainability.

Consider documenting the enum with JSDoc comments to describe each node type's purpose, especially for less common ones like CDATA_SECTION_NODE and PROCESSING_INSTRUCTION_NODE. This would help other developers understand when to use each type.


345-345: Consider enhancing void element detection.

While the update to use NodeType.ELEMENT_NODE is good, consider these improvements:

  1. Extract the additional void elements (iframe, video) into a constant array alongside htmlVoidElements
  2. Consider making the iteration limit (10000) configurable or documenting why this specific number was chosen
+ const ADDITIONAL_VOID_ELEMENTS = ['iframe', 'video']
+ const MAX_DEPTH_ITERATIONS = 10000 // Prevent infinite loops in malformed DOM

  export function getFirstVoidChild(elem: DOMElement): DOMElement | null {
    // ...
    num += 1
-   if (num > 10000) { break }
+   if (num > MAX_DEPTH_ITERATIONS) { break }
    // ...
    if (
-     htmlVoidElements.includes(name)
-     // 补充一些
-     || name === 'iframe'
-     || name === 'video'
+     htmlVoidElements.includes(name) || ADDITIONAL_VOID_ELEMENTS.includes(name)
    ) {

391-394: Improve readability of the loop condition.

While the update to use NodeType enum values is good, the loop condition contains an assignment which could be confusing.

Consider restructuring the loop to make the assignment more explicit:

- for (let nodes = elem.childNodes, i = nodes.length; i -= 1;) {
+ const nodes = elem.childNodes
+ for (let i = nodes.length - 1; i >= 0; i--) {
🧰 Tools
🪛 Biome

[error] 394-394: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7b0bb58 and 7914f56.

📒 Files selected for processing (5)
  • packages/core/src/editor/plugins/with-content.ts (15 hunks)
  • packages/core/src/editor/plugins/with-max-length.ts (4 hunks)
  • packages/core/src/parse-html/parse-common-elem-html.ts (9 hunks)
  • packages/core/src/parse-html/parse-elem-html.ts (2 hunks)
  • packages/core/src/utils/dom.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/core/src/editor/plugins/with-content.ts
🧰 Additional context used
🪛 Biome
packages/core/src/utils/dom.ts

[error] 394-394: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

🔇 Additional comments (9)
packages/core/src/parse-html/parse-elem-html.ts (1)

11-11: LGTM! Good refactoring of imports.

The change from NodeType to isDOMText import reflects a move towards more semantic utility functions, which improves code maintainability.

packages/core/src/editor/plugins/with-max-length.ts (2)

6-12: LGTM! Import organization and documentation improvements.

The added comment about IME composition handling is valuable documentation, and the import reorganization improves code readability.


Line range hint 74-98: Consider enhancing fragment insertion with partial content support.

The current implementation handles the first node specially but rejects subsequent nodes entirely if they exceed the limit. Consider implementing a more graceful degradation that preserves as much content as possible within the maxLength constraint.

Let's verify if this pattern exists in other editor implementations:

packages/core/src/parse-html/parse-common-elem-html.ts (4)

7-14: LGTM! Well-organized imports.

The imports are now logically grouped, improving code readability and dependency management.


71-71: LGTM! Improved type checking with utility functions.

Replacing direct nodeType checks with isDOMElement and isDOMText utility functions enhances type safety and code readability.

Also applies to: 95-95


127-127: LGTM! Clear indication of unused parameters.

Using underscore prefix for unused parameters follows TypeScript conventions and makes the code intent clearer.


160-160: LGTM! Consistent use of curly braces.

Adding curly braces to the if statement improves code consistency and reduces potential errors in future modifications.

packages/core/src/utils/dom.ts (2)

139-139: Consistent use of NodeType enum across node type checks!

The changes improve code readability by replacing magic numbers with meaningful enum values.

Also applies to: 146-146, 160-160


Line range hint 408-408: Clean usage of NodeType enum!

The function properly handles edge cases and consistently uses the NodeType enum.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (3)
scripts/build-all.sh (2)

3-4: Translate comments to English for better maintainability

Consider translating the Chinese comment to English to maintain consistency and improve accessibility for all contributors.

-## 一键打包所有 package
+## Build all packages in one command

1-47: Add script documentation and package dependency documentation

The script would benefit from better documentation about the build order requirements and package dependencies.

Add this documentation at the start of the file:

 #!/bin/bash
 
+# Build All Packages Script
+# ------------------------
+# This script builds all packages in the correct dependency order:
+# 
+# Build Order and Dependencies:
+# 1. core (required by all)
+# 2. basic-modules
+# 3. code-highlight (depends on basic-modules/code-block)
+# 4. list-module
+# 5. table-module
+# 6. upload-image-module (depends on basic-modules/image)
+# 7. video-module
+# 8. editor (depends on core and all modules)
+#
+# Usage: ./build-all.sh [dev|build]
+#   - dev: Development build
+#   - build: Production build (default)
🧰 Tools
🪛 Shellcheck

[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 14-14: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 18-18: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 23-23: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 31-31: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 36-36: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 40-40: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 45-45: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

packages/core/src/utils/dom.ts (1)

112-114: Add JSDoc documentation for the new function.

The isUnprocessedListElement function is well-implemented, but would benefit from JSDoc documentation explaining its purpose, parameters, and return value.

Add documentation like this:

+/**
+ * Checks if an element is an unprocessed list element (ul/ol without data-w-e-type attribute)
+ * @param el - The element to check
+ * @returns boolean - True if the element is an unprocessed list element
+ */
 export const isUnprocessedListElement = (el: Element): boolean => {
   return 'matches' in el && el.matches('ul:not([data-w-e-type]),ol:not([data-w-e-type])')
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7914f56 and 296580f.

📒 Files selected for processing (7)
  • package.json (1 hunks)
  • packages/core/src/editor/dom-editor.ts (2 hunks)
  • packages/core/src/editor/plugins/with-content.ts (15 hunks)
  • packages/core/src/render/helper.ts (1 hunks)
  • packages/core/src/utils/dom.ts (6 hunks)
  • packages/custom-types.d.ts (1 hunks)
  • scripts/build-all.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/core/src/editor/plugins/with-content.ts
🧰 Additional context used
🪛 Biome
packages/core/src/utils/dom.ts

[error] 397-397: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

🪛 Shellcheck
scripts/build-all.sh

[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 14-14: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 18-18: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 23-23: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 31-31: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 36-36: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 40-40: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 45-45: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

🔇 Additional comments (9)
packages/core/src/render/helper.ts (3)

6-6: LGTM! Good use of shared types.

The import of ElementType from a shared package promotes consistency across the codebase.


8-9: LGTM! Well-structured type-safe implementation.

The function changes look good:

  • Uses ElementType for type safety (addressing the previous review comment)
  • Maintains a clear and consistent ID format
  • Keeps the function simple and focused

8-9: Verify all callers have been updated.

Since this is a core utility function with a modified signature, let's verify all callers have been updated correctly.

packages/custom-types.d.ts (1)

68-69: LGTM! Good addition for type safety.

The ElementType type alias provides proper type safety for element type strings, which is particularly useful for the updated genElemId function.

Let's verify the usage of this type in the codebase:

✅ Verification successful

ElementType is correctly used and integrated

The verification shows that:

  • ElementType is properly imported and used in genElemId function in core package
  • The table module has its own scoped ElementType definition which is independent
  • The implementation aligns with the type safety goals
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the ElementType usage in genElemId function and other potential consumers

# Test 1: Check if genElemId is using the new ElementType
ast-grep --pattern 'function genElemId($_: $_, type: ElementType)'

# Test 2: Look for other potential consumers of ElementType
rg -l "ElementType" --type ts

Length of output: 203


Script:

#!/bin/bash
# Let's check the actual implementation and usage of ElementType in the found files

# Check the implementation in helper.ts
rg -A 5 "ElementType" packages/core/src/render/helper.ts

# Check the implementation in options.ts
rg -A 5 "ElementType" packages/table-module/src/utils/options.ts

# Look for genElemId function implementation
ast-grep --pattern 'function genElemId($$$)'

Length of output: 738

package.json (1)

13-15: Clarify the build strategy with parallel scripts.

The repository now has two development scripts:

  • "dev": "turbo run dev"
  • "dev:sh": "sh scripts/build-all.sh dev"

Please document the differences and when to use each one.

#!/bin/bash
# Check if turbo.json exists and examine its configuration
if [ -f "turbo.json" ]; then
    echo "Turbo configuration:"
    cat turbo.json
fi
packages/core/src/utils/dom.ts (2)

87-96: Well-structured enum for DOM node types!

Good addition of the NodeType enum to replace magic numbers with named constants. The values correctly match the W3C DOM specification, improving code readability and maintainability.


142-142: Good replacement of magic numbers with NodeType enum!

The consistent use of the NodeType enum across node type checking functions improves code maintainability.

Also applies to: 149-149, 163-163, 348-348

packages/core/src/editor/dom-editor.ts (2)

25-25: LGTM! Import addition enhances code consistency.

The addition of isDOMText import aligns with the codebase's pattern of using dedicated DOM utility functions.


Line range hint 737-741: LGTM! Improved text node check using utility function.

The replacement of direct nodeType check with isDOMText utility:

  • Enhances code readability
  • Maintains consistency with other DOM checks
  • Makes the code more maintainable

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 296580f and af62f69.

📒 Files selected for processing (1)
  • packages/core/src/utils/dom.ts (6 hunks)
🔇 Additional comments (3)
packages/core/src/utils/dom.ts (3)

87-96: Well-structured enum for DOM node types!

Good addition of the NodeType enum to replace magic numbers. This improves code readability and maintainability by providing meaningful constants for DOM node types.


142-142: LGTM! Good use of NodeType enum.

The updates to use NodeType enum constants in type checking functions improve code clarity while maintaining the same functionality.

Also applies to: 149-149, 163-163


394-397: LGTM! Good implementation of the suggested changes.

The node type checking has been simplified and improved by:

  1. Using isDOMText helper for text node check
  2. Using array includes with NodeType enum for other node types

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
scripts/build-base.sh (2)

15-21: Improve variable initialization order and remove extra whitespace.

The build start time should be captured before the directory change to ensure it's always recorded, even if the script fails early.

Apply these changes:

+# Initialize build status tracking
+declare -A build_status
+build_start_time=$(date +%s)
+
 cd ./packages || { echo "Error: packages directory not found"; exit 1; }
-
-
-# Initialize build status tracking
-declare -A build_status
-build_start_time=$(date +%s)

63-74: Improve package build configuration and flexibility.

The current implementation has several limitations:

  1. Package dependencies are implied by build order but not verified
  2. No ability to build specific packages
  3. Package list is hard-coded

Consider these improvements:

  1. Add a configuration file for package definitions:
{
  "packages": {
    "core": { "dependencies": [] },
    "basic-modules": { "dependencies": ["core"] },
    "editor": { "dependencies": ["core", "basic-modules"] }
  }
}
  1. Modify the script to read from this configuration:
+# Read package configuration
+config_file="package-build-config.json"
+if [ -f "$config_file" ]; then
+    packages=$(jq -r '.packages | keys[]' "$config_file")
+else
+    echo "Error: Missing $config_file"
+    exit 1
+fi
+
+# Allow building specific packages
+if [ -n "$2" ]; then
+    requested_package=$2
+    if echo "$packages" | grep -q "^$requested_package$"; then
+        build_package "$requested_package"
+        exit $?
+    else
+        echo "Error: Package $requested_package not found"
+        exit 1
+    fi
+fi
+
-# Build packages in dependency order
-build_package "core"
-build_package "basic-modules"
-build_package "code-highlight"
-build_package "list-module"
-build_package "table-module"
-build_package "upload-image-module"
-build_package "video-module"
-build_package "editor"
+# Build all packages in dependency order
+for package in $packages; do
+    build_package "$package"
+done
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between af62f69 and 425aac8.

📒 Files selected for processing (3)
  • package.json (1 hunks)
  • packages/core/src/utils/dom.ts (6 hunks)
  • scripts/build-base.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • packages/core/src/utils/dom.ts

@cycleccc cycleccc force-pushed the 25-功能提供更灵活的编辑区样式 branch from b1f8219 to 425aac8 Compare October 29, 2024 02:51
@cycleccc cycleccc merged commit f1848d5 into master Oct 29, 2024
2 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Nov 9, 2024
5 tasks
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.

功能:根据 id 匹配自定义样式(已在 wangEditor-next 实现)
1 participant