Merged
Conversation
Reviewer's Guide by SourceryThis pull request updates the Updated class diagram for PaneviewPanel and DraggablePaneviewPanelclassDiagram
class PaneviewPanel {
- headerSize: number
- _orthogonalSize: number
- _size: number
- _minimumBodySize: number
- _maximumBodySize: number
- _isExpanded: boolean
+ onDidChangeExpansionState: Event
+ onDidChange: Event
+ isExpanded(): boolean
+ layout(width: number, height: number): void
+ init(parameters: any): void
}
note for PaneviewPanel "Constructor now takes an options object"
class DraggablePaneviewPanel {
- accessor: any
+ onDidDrop: Event
+ onUnhandledDragOverEvent: Event
}
note for DraggablePaneviewPanel "Constructor now takes an options object"
PaneviewPanel <|-- DraggablePaneviewPanel
Updated class diagram for DockviewComponentclassDiagram
class DockviewComponent {
-_onDidPopoutGroupSizeChange: Emitter
+onDidPopoutGroupSizeChange: Event
-_onDidPopoutGroupPositionChange: Emitter
+onDidPopoutGroupPositionChange: Event
+orthogonalize(position: string, options?: any): void
}
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a consistent naming scheme for CSS variables (e.g., prefixing all of them with
--bb-dockview-to avoid potential conflicts). - It looks like you're adding new events to
DockviewComponent; ensure these events are properly documented and that their usage is clear.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
| } | ||
| const DEBOUCE_DELAY = 100; |
There was a problem hiding this comment.
nitpick (typo): Possible typo in constant name.
Consider renaming 'DEBOUCE_DELAY' to 'DEBOUNCE_DELAY' to improve clarity and reduce potential confusion.
Suggested change
| const DEBOUCE_DELAY = 100; | |
| const DEBOUNCE_DELAY = 100; |
| }), _onDidWindowPositionChange.event(() => { | ||
| this._onDidPopoutGroupPositionChange.fire({ | ||
| screenX: _window.window.screenX, | ||
| screenY: _window.window.screenX, |
There was a problem hiding this comment.
issue (bug_risk): Potential incorrect property used in event payload.
The 'screenY' property is being set with '_window.window.screenX'; verify if this is intentional or if it should use '_window.window.screenY'.
| } | ||
| return true; | ||
| } | ||
| function onDidWindowMoveEnd(window) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the debouncing and recursive shadow DOM logic into small, clearly named helper functions to improve readability and reduce nested control flow complexity.
Consider extracting both the debouncing and recursive shadow DOM logic into clearly named, small helper functions. This will reduce nested control flow and improve readability.
For example, instead of embedding debouncing logic directly in `onDidWindowMoveEnd`, create a debounce helper:
```js
// Option 1: Use a lightweight debounce utility from a library like lodash
import debounce from 'lodash.debounce';
// Or a simple custom debounce helper:
function debounceFn(fn, delay) {
let timer;
return (...args) => {
clearTimeout(timer);
timer = setTimeout(() => fn(...args), delay);
};
}
function onDidWindowMoveEnd(window) {
const emitter = new Emitter();
let previousX = window.screenX;
let previousY = window.screenY;
const fireEmitter = debounceFn(() => emitter.fire(), DEBOUCE_DELAY);
function checkMovement() {
if (window.closed) return;
const { screenX, screenY } = window;
if (screenX !== previousX || screenY !== previousY) {
fireEmitter();
previousX = screenX;
previousY = screenY;
}
requestAnimationFrame(checkMovement);
}
checkMovement();
return emitter;
}Similarly, consider separating the recursive shadow DOM traversal:
function collectMatchingElements(node, tagNames, result = []) {
if (node.nodeType === Node.ELEMENT_NODE) {
if (tagNames.includes(node.tagName)) {
result.push(node);
}
if (node.shadowRoot) {
collectMatchingElements(node.shadowRoot, tagNames, result);
}
for (const child of node.children) {
collectMatchingElements(child, tagNames, result);
}
}
return result;
}
function allTagsNamesInclusiveOfShadowDoms(tagNames) {
return collectMatchingElements(document.documentElement, tagNames);
}By isolating these concerns, the overall flow is flatter and easier to follow while keeping functionality intact.
Comment on lines
+521
to
+533
| function findIframesInNode(node) { | ||
| if (node.nodeType === Node.ELEMENT_NODE) { | ||
| if (tagNames.includes(node.tagName)) { | ||
| iframes.push(node); | ||
| } | ||
| if (node.shadowRoot) { | ||
| findIframesInNode(node.shadowRoot); | ||
| } | ||
| for (const child of node.children) { | ||
| findIframesInNode(child); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
Comment on lines
+631
to
+637
| const disposable = new CompositeDisposable(addDisposableListener(element, 'resize', () => { | ||
| clearTimeout(resizeTimeout); | ||
| resizeTimeout = setTimeout(() => { | ||
| cb(); | ||
| }, DEBOUCE_DELAY); | ||
| })); | ||
| return disposable; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const disposable = new CompositeDisposable(addDisposableListener(element, 'resize', () => { | |
| clearTimeout(resizeTimeout); | |
| resizeTimeout = setTimeout(() => { | |
| cb(); | |
| }, DEBOUCE_DELAY); | |
| })); | |
| return disposable; | |
| return new CompositeDisposable(addDisposableListener(element, 'resize', () => { | |
| clearTimeout(resizeTimeout); | |
| resizeTimeout = setTimeout(() => { | |
| cb(); | |
| }, DEBOUCE_DELAY); | |
| })); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #373
Summary By Copilot
This pull request includes several updates and improvements to the
BootstrapBlazor.DockViewcomponent, focusing on version updates, CSS adjustments, and JavaScript code refactoring. The most important changes include updating the component version, refining CSS styles, and enhancing JavaScript functions for better performance and maintainability.Version Update:
BootstrapBlazor.DockViewfrom9.1.3to9.1.4in thecsprojfile.CSS Adjustments:
dockview-bb.css..dv-resize-container-drawerclass to100%indockview-bb.css..dv-default-tab-actionvisible in inactive tabs indockview-bb.css..dv-watermarkstyling todockview.cssfor better organization. [1] [2]JavaScript Refactoring:
dockview-corefrom4.0.1to4.1.0indockview-core.esm.js.addDisposableWindowListenerandgetElementsByTagNamefunctions, and replaced their usages withaddDisposableListenerfor consistency. [1] [2] [3] [4] [5] [6] [7] [8] [9]disableIframePointEventsfunction to efficiently handle shadow DOMs.onDidWindowMoveEndandonDidWindowResizeEndfunctions for better event handling.PaneviewPanelandDraggablePaneviewPanelconstructors for better readability and maintainability. [1] [2]onDidPopoutGroupSizeChangeandonDidPopoutGroupPositionChangetoDockviewComponentfor enhanced functionality. [1] [2] [3]Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge