Various merge tags related improvements and adjustments. #16096
Conversation
Internal (merge-tags): Added a mergeTag element to the `inlineObjectElements` array in the `DomConverter` class.
Feature (ui): Add the `LabelWithHighlightView` and `ButtonLabelWithHighlightView` components to the common UI library. Feature (ui): Add the `filterGroupAndItemNames()` helper to the common UI library. Feature (ui): The `SearchTextView#reset()` method will also reset the scroll of its `filteredView` to the top.
Internal (merge-tags): Add the merge tag-related menu bar items.
WalkthroughThe recent updates across the CKEditor 5 package focus on enhancing UI components and integration with new elements like Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
packages/ckeditor5-ui/src/highlightedtext/labelwithhighlightview.ts (1)
21-26: Ensure proper documentation for public propertiesidandfor.It's good practice to include a brief description in the documentation comments for public properties to explain their purpose or usage, which can be beneficial for other developers.
packages/ckeditor5-ui/src/highlightedtext/buttonlabelwithhighlightview.ts (1)
20-30: Ensure proper documentation for public propertiesstyle,text, andid.Similar to the previous file, it would be beneficial to include brief descriptions in the documentation comments for these public properties to aid other developers in understanding their usage.
| @@ -182,7 +182,7 @@ export default class DomConverter { | |||
| 'td', 'tfoot', 'th', 'thead', 'tr', 'ul' | |||
| ]; | |||
| this.inlineObjectElements = [ | |||
| 'object', 'iframe', 'input', 'button', 'textarea', 'select', 'option', 'video', 'embed', 'audio', 'img', 'canvas' | |||
| 'object', 'iframe', 'input', 'button', 'textarea', 'select', 'option', 'video', 'embed', 'audio', 'img', 'canvas', 'mergeTag' | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Several files in the codebase still use the old function signature.
- The search results indicate that 'mergeTag' is referenced in UI-related code but does not appear to be handled in a way that supports its behavior as an inline object element in
domconverter.ts. - This could potentially lead to issues since the handling necessary to support its inclusion in the
inlineObjectElementsarray does not seem to be implemented.
Analysis chain
Addition of 'mergeTag' to inlineObjectElements approved.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'mergeTag' is handled appropriately in other parts of the codebase.
# Test: Search for 'mergeTag' handling. Expect: Relevant handling logic in other parts of the code.
rg --type typescript 'mergeTag'
Length of output: 66
Script:
#!/bin/bash
# Description: Re-verify that 'mergeTag' is handled appropriately in other parts of the codebase by correcting the file type error in the previous script.
# Corrected Test: Search for 'mergeTag' handling in TypeScript files. Expect: Relevant handling logic in other parts of the code.
rg --type ts 'mergeTag'
Length of output: 511
There was a problem hiding this comment.
This should be injected from the feature code. It's public property. The property itself is read-only but the content is not.
See how we handle other similar props:
There was a problem hiding this comment.
@niegowski, thank you for your input. The handling of 'mergeTag' as an inline object element in the DomConverter class is indeed crucial for ensuring it integrates smoothly with the rest of the system, especially given its role in the UI components.
From the examples you provided, it seems like the approach used in other parts of CKEditor 5 involves defining properties in a way that they are publicly accessible but their modification is controlled. This is typically achieved using getter/setter methods or similar patterns to encapsulate the property's value, ensuring it remains read-only externally while allowing its content to be modified internally.
Currently, 'mergeTag' is added directly to the inlineObjectElements array in the DomConverter class, which makes it part of the internal configuration for handling inline objects. This approach is straightforward but does not allow external manipulation or extension in the same way that a getter/setter pattern would.
If aligning the handling of 'mergeTag' with the approach you highlighted is desired, we could consider introducing a method to get and potentially set the contents of the inlineObjectElements array in a controlled manner. This would provide flexibility and maintain consistency with other parts of the codebase.
Would you like me to draft a potential implementation for this adjustment, or is there a specific aspect of the integration you would like to discuss further?
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
packages/ckeditor5-ui/src/panel/balloon/balloonpanelview.ts (2)
401-421: Add JSDoc for theoptionsparameter.To improve code readability and maintainability, consider adding JSDoc comments for the
optionsparameter in thegeneratePositionsmethod./** * @param {Object} options - Options to generate positions. * @param {number} [options.sideOffset] - A custom side offset (in pixels) of each position. * @param {number} [options.heightOffset] - A custom height offset (in pixels) of each position. * @param {number} [options.stickyVerticalOffset] - A custom offset (in pixels) of the `viewportStickyNorth` positioning function. * @param {Object} [options.config] - Additional configuration of the balloon panel view. */
437-441: Ensure consistent use of configuration spread.For consistency, ensure that the spread operator is used uniformly across all positioning functions when adding the
configobject....( config && { config } )
| viewportStickyNorth: ( targetRect, balloonRect, viewportRect, limiterRect ) => { | ||
| const boundaryRect = limiterRect || viewportRect; | ||
|
|
||
| if ( !targetRect.getIntersection( boundaryRect ) ) { | ||
| return null; | ||
| } | ||
|
|
||
| // Engage when the target top and bottom edges are close or off the boundary. | ||
| // By close, it means there's not enough space for the balloon arrow (offset). | ||
| if ( boundaryRect.height - targetRect.height > stickyVerticalOffset ) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| top: boundaryRect.top + stickyVerticalOffset, | ||
| left: targetRect.left + targetRect.width / 2 - balloonRect.width / 2, | ||
| name: 'arrowless', | ||
| config: { | ||
| withArrow: false, | ||
| ...config | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Consider adding a fallback for stickyVerticalOffset.
To enhance robustness, consider adding a fallback value for stickyVerticalOffset in the viewportStickyNorth positioning function.
const stickyOffset = stickyVerticalOffset || BalloonPanelView.stickyVerticalOffset;
Dumluregn
force-pushed
the
cc/5988-merge-tags
branch
from
92dd882
to
7590c11
Compare
Suggested merge commit message (convention)
Feature (ui): Add the
LabelWithHighlightViewandButtonLabelWithHighlightViewcomponents to the common UI library.Feature (ui): Add the
filterGroupAndItemNames()helper to the common UI library.Feature (ui): The
SearchTextView#reset()method will also reset the scroll of itsfilteredViewto the top.Internal (merge-tags): Added a
mergeTagelement to theinlineObjectElementsarray in theDomConverterclass.Additional information
This is a PR with improvements necessary for the upcoming merge tags feature.