Merged
Conversation
Reviewer's GuideThis PR upgrades dockview core to v4.5.0, enabling runtime toggling of drag-and-drop via the new disableDnd option with propagated state updates, introduces a skipSetActive flag throughout panel move APIs for consistent active state handling, and refines CSS (box-sizing and tab styling) while removing duplicate rules. Class diagram for Dockview drag-and-drop and active state updatesclassDiagram
class DockviewComponent {
+options
+updateDragAndDropState()
+updateOptions(options)
}
class DockviewGroupPanelModel {
+tabsContainer
+updateDragAndDropState()
}
class TabsContainer {
+tabs
+voidContainer
+updateDragAndDropState()
}
class Tabs {
+_tabs
+updateDragAndDropState()
}
class Tab {
+_element
+updateDragAndDropState()
}
class VoidContainer {
+_element
+updateDragAndDropState()
}
DockviewComponent --> "*" DockviewGroupPanelModel : groups
DockviewGroupPanelModel --> TabsContainer : tabsContainer
TabsContainer --> Tabs : tabs
TabsContainer --> VoidContainer : voidContainer
Tabs --> Tab : _tabs
Class diagram for moveTo and skipSetActive propagationclassDiagram
class DockviewComponent {
+moveGroupOrPanel(options)
}
class DockviewGroupPanelApiImpl {
+moveTo(options)
}
class DockviewPanelApiImpl {
+moveTo(options)
}
DockviewGroupPanelApiImpl --|> DockviewComponent : uses moveGroupOrPanel
DockviewPanelApiImpl --|> DockviewComponent : uses moveGroupOrPanel
DockviewGroupPanelApiImpl : +skipSetActive
DockviewPanelApiImpl : +skipSetActive
DockviewComponent : +skipSetActive
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for your PR, @zhaijunlei955. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @zhaijunlei955 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-core.esm.js:4993` </location>
<code_context>
this._element.className = 'dv-tab';
this._element.tabIndex = 0;
- this._element.draggable = true;
+ this._element.draggable = !this.accessor.options.disableDnd;
toggleClass(this.element, 'dv-inactive-tab', true);
const dragHandler = new TabDragHandler(this._element, this.accessor, this.group, this.panel);
</code_context>
<issue_to_address>
Directly toggling draggable based on disableDnd may cause accessibility issues.
Review whether additional ARIA attributes or focus management are needed to maintain accessibility when drag-and-drop is disabled.
Suggested implementation:
```javascript
this._element = document.createElement('div');
this._element.className = 'dv-tab';
this._element.tabIndex = 0;
this._element.draggable = !this.accessor.options.disableDnd;
if (!this.accessor.options.disableDnd) {
this._element.setAttribute('aria-grabbed', 'false');
this._element.setAttribute('aria-dropeffect', 'move');
} else {
this._element.setAttribute('aria-grabbed', 'false');
this._element.removeAttribute('aria-dropeffect');
}
toggleClass(this.element, 'dv-inactive-tab', true);
const dragHandler = new TabDragHandler(this._element, this.accessor, this.group, this.panel);
```
```javascript
updateDragAndDropState() {
this._element.draggable = !this.accessor.options.disableDnd;
if (!this.accessor.options.disableDnd) {
this._element.setAttribute('aria-grabbed', 'false');
this._element.setAttribute('aria-dropeffect', 'move');
} else {
this._element.setAttribute('aria-grabbed', 'false');
this._element.removeAttribute('aria-dropeffect');
}
}
```
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-core.esm.js:9885` </location>
<code_context>
+ // Calculate position based on available properties
+ let left, top;
+ if ('left' in box) {
+ left = box.left + 50;
+ }
+ else if ('right' in box) {
+ left = Math.max(0, box.right - box.width - 50);
+ }
+ else {
+ left = 50; // Default fallback
+ }
+ if ('top' in box) {
</code_context>
<issue_to_address>
Default fallback positions for floating groups may cause overlap.
A static fallback of 50 for left/top may cause overlapping windows if several groups lack valid box properties. Consider dynamically adjusting the fallback to prevent overlap, such as incrementing the value or checking for free space.
Suggested implementation:
```javascript
let left, top;
if ('left' in box) {
left = box.left + 50;
}
else if ('right' in box) {
left = Math.max(0, box.right - box.width - 50);
}
else {
// Dynamically offset fallback to avoid overlap
if (!this._floatingFallbackCount) {
this._floatingFallbackCount = 0;
}
left = 50 + (this._floatingFallbackCount * 40);
}
if ('top' in box) {
top = box.top + 50;
}
else if ('bottom' in box) {
top = Math.max(0, box.bottom - box.height - 50);
}
else {
// Dynamically offset fallback to avoid overlap
if (!this._floatingFallbackCount) {
this._floatingFallbackCount = 0;
}
top = 50 + (this._floatingFallbackCount * 40);
this._floatingFallbackCount++;
}
```
- You may want to reset `this._floatingFallbackCount` when all floating groups are closed or repositioned, to avoid the counter growing indefinitely.
- If fallback is used for both left and top, incrementing the counter only once per group (as above) is usually sufficient, but you may want to fine-tune the offset logic for your UI.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.DockView/wwwroot/css/dockview.css:1071` </location>
<code_context>
+ white-space: nowrap;
+ text-overflow: ellipsis;
+}
+.dv-tab .dv-default-tab .dv-default-tab-content {
+ flex-grow: 1;
+ margin-right: 4px;
+}
+.dv-tab .dv-default-tab .dv-default-tab-action {
</code_context>
<issue_to_address>
Hardcoded margin may not scale with different font sizes or themes.
Consider replacing the hardcoded 4px margin with a relative unit like em/rem or a CSS variable to improve scalability across themes and font sizes.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
.dv-tab .dv-default-tab .dv-default-tab-content {
flex-grow: 1;
margin-right: 4px;
}
=======
:root {
--dv-tab-margin-right: 0.5em;
}
.dv-tab .dv-default-tab .dv-default-tab-content {
flex-grow: 1;
margin-right: var(--dv-tab-margin-right);
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| this._element.className = 'dv-tab'; | ||
| this._element.tabIndex = 0; | ||
| this._element.draggable = true; | ||
| this._element.draggable = !this.accessor.options.disableDnd; |
There was a problem hiding this comment.
suggestion: Directly toggling draggable based on disableDnd may cause accessibility issues.
Review whether additional ARIA attributes or focus management are needed to maintain accessibility when drag-and-drop is disabled.
Suggested implementation:
this._element = document.createElement('div');
this._element.className = 'dv-tab';
this._element.tabIndex = 0;
this._element.draggable = !this.accessor.options.disableDnd;
if (!this.accessor.options.disableDnd) {
this._element.setAttribute('aria-grabbed', 'false');
this._element.setAttribute('aria-dropeffect', 'move');
} else {
this._element.setAttribute('aria-grabbed', 'false');
this._element.removeAttribute('aria-dropeffect');
}
toggleClass(this.element, 'dv-inactive-tab', true);
const dragHandler = new TabDragHandler(this._element, this.accessor, this.group, this.panel); updateDragAndDropState() {
this._element.draggable = !this.accessor.options.disableDnd;
if (!this.accessor.options.disableDnd) {
this._element.setAttribute('aria-grabbed', 'false');
this._element.setAttribute('aria-dropeffect', 'move');
} else {
this._element.setAttribute('aria-grabbed', 'false');
this._element.removeAttribute('aria-dropeffect');
}
}
Comment on lines
+9885
to
+9891
| left = box.left + 50; | ||
| } | ||
| else if ('right' in box) { | ||
| left = Math.max(0, box.right - box.width - 50); | ||
| } | ||
| else { | ||
| left = 50; // Default fallback |
There was a problem hiding this comment.
suggestion (bug_risk): Default fallback positions for floating groups may cause overlap.
A static fallback of 50 for left/top may cause overlapping windows if several groups lack valid box properties. Consider dynamically adjusting the fallback to prevent overlap, such as incrementing the value or checking for free space.
Suggested implementation:
let left, top;
if ('left' in box) {
left = box.left + 50;
}
else if ('right' in box) {
left = Math.max(0, box.right - box.width - 50);
}
else {
// Dynamically offset fallback to avoid overlap
if (!this._floatingFallbackCount) {
this._floatingFallbackCount = 0;
}
left = 50 + (this._floatingFallbackCount * 40);
}
if ('top' in box) {
top = box.top + 50;
}
else if ('bottom' in box) {
top = Math.max(0, box.bottom - box.height - 50);
}
else {
// Dynamically offset fallback to avoid overlap
if (!this._floatingFallbackCount) {
this._floatingFallbackCount = 0;
}
top = 50 + (this._floatingFallbackCount * 40);
this._floatingFallbackCount++;
}- You may want to reset
this._floatingFallbackCountwhen all floating groups are closed or repositioned, to avoid the counter growing indefinitely. - If fallback is used for both left and top, incrementing the counter only once per group (as above) is usually sufficient, but you may want to fine-tune the offset logic for your UI.
Comment on lines
+1071
to
+1074
| .dv-tab .dv-default-tab .dv-default-tab-content { | ||
| flex-grow: 1; | ||
| margin-right: 4px; | ||
| } |
There was a problem hiding this comment.
suggestion: Hardcoded margin may not scale with different font sizes or themes.
Consider replacing the hardcoded 4px margin with a relative unit like em/rem or a CSS variable to improve scalability across themes and font sizes.
Suggested change
| .dv-tab .dv-default-tab .dv-default-tab-content { | |
| flex-grow: 1; | |
| margin-right: 4px; | |
| } | |
| :root { | |
| --dv-tab-margin-right: 0.5em; | |
| } | |
| .dv-tab .dv-default-tab .dv-default-tab-content { | |
| flex-grow: 1; | |
| margin-right: var(--dv-tab-margin-right); | |
| } |
| })); | ||
| this.doSetGroupAndPanelActive(destinationGroup); | ||
| this.movingLock(() => { | ||
| var _a; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
ArgoZhang
changed the title
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 #494
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Upgrade dockview to version 4.5.0 with support for dynamically enabling/disabling drag-and-drop, optional suppression of active state changes during moves, and refined styling and state-refresh capabilities across components.
New Features:
Enhancements: