-
Notifications
You must be signed in to change notification settings - Fork 1
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(DockView): add ShowPin parameter #155
Conversation
Reviewer's Guide by SourceryThis PR adds a new ShowPin parameter to the DockView component, implementing a pin/auto-hide functionality for floating panels. The changes include significant updates to the docking system's UI behavior, panel management, and styling. Sequence diagram for panel pinning and auto-hide functionalitysequenceDiagram
participant User
participant DockView
participant Panel
participant Group
User->>DockView: Click pin button
DockView->>Panel: Toggle pin state
Panel->>Group: Update visibility
Group->>DockView: Adjust layout
DockView->>User: Update UI with new panel state
Class diagram for DockViewConfig with ShowPin parameterclassDiagram
class DockViewConfig {
bool ShowClose
bool ShowPin
bool ShowMaximize
}
note for DockViewConfig "ShowPin is a new parameter added to control the visibility of the pin button."
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing or restoring the commented out code sections to improve maintainability and reduce confusion. If the code is no longer needed, it should be deleted rather than left commented out.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues 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.
src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-extensions.js
Show resolved
Hide resolved
arialLevelTracker.push(this._element); | ||
} | ||
bringToFront() { | ||
arialLevelTracker.push(this._element); | ||
} | ||
setBounds(bounds = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider extracting overlay positioning logic into a reusable helper function.
The Overlay positioning logic could be simplified by extracting the common positioning calculations into a helper function. This would reduce code duplication and improve maintainability. Here's a suggested approach:
private calculatePosition(bounds: {
top?: number;
left?: number;
bottom?: number;
right?: number;
width?: number;
height?: number;
}) {
const containerRect = this.options.container.getBoundingClientRect();
const overlayRect = this._element.getBoundingClientRect();
const result = { ...bounds };
if ('top' in bounds) {
result.top = Math.max(0, bounds.top);
result.bottom = 'auto';
this.verticalAlignment = 'top';
} else if ('bottom' in bounds) {
result.bottom = Math.max(0, bounds.bottom);
result.top = 'auto';
this.verticalAlignment = 'bottom';
}
if ('left' in bounds) {
result.left = Math.max(0, bounds.left);
result.right = 'auto';
this.horizontalAlignment = 'left';
} else if ('right' in bounds) {
result.right = Math.max(0, bounds.right);
result.left = 'auto';
this.horizontalAlignment = 'right';
}
return result;
}
Then simplify setBounds() and setupDrag() to use this helper:
setBounds(bounds = {}) {
const position = this.calculatePosition(bounds);
Object.entries(position).forEach(([key, value]) => {
this._element.style[key] = value === 'auto' ? value : `${value}px`;
});
this._onDidChange.fire();
}
This refactoring:
- Centralizes position calculation logic
- Reduces code duplication
- Makes the positioning behavior more maintainable
- Preserves all existing functionality including boundary checks
@@ -74,3 +75,50 @@ | |||
} | |||
} | |||
} | |||
|
|||
const setVisible = DockviewComponent.prototype.setVisible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider splitting the setVisible method into smaller focused functions for better separation of concerns
The setVisible method mixes multiple concerns and uses nested conditionals. Consider extracting the z-index and sash management into focused helper functions:
function updateGroupZIndex(groupElement, isVisible) {
groupElement.parentElement.style.zIndex = isVisible ? 'initial' : '-1';
}
function updateSashesForBranch(branch, isVisible) {
const {orientation, splitview: {sashes}} = branch;
const sizeStr = branch.size - 2 + 'px';
sashes?.forEach(sash => {
const shouldHide = !isVisible && (
(sash.container.style.left === '0px' && sash.container.style.top === '0px') ||
(orientation === 'HORIZONTAL' && sash.container.style.left === sizeStr) ||
(orientation === 'VERTICAL' && sash.container.style.top === sizeStr)
);
sash.container.style.zIndex = shouldHide ? '-1' : '99';
});
}
DockviewComponent.prototype.setVisible = function(...args) {
setVisible.apply(this, args);
const [group, isVisible] = args;
const branch = getBranchByGroup(group);
updateGroupZIndex(group.element, isVisible);
updateSashesForBranch(branch, isVisible);
}
This refactoring:
- Separates z-index management from sash positioning logic
- Reduces nesting by extracting conditional logic
- Makes the code more maintainable and testable
- Preserves all existing functionality
const json = dockview.toJSON(); | ||
if(dockview.floatingGroups && dockview.floatingGroups.length > 0) { | ||
json.floatingGroups.forEach((fg, index) => { | ||
const width = dockview.floatingGroups[index].group.width |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring
)
const width = dockview.floatingGroups[index].group.width | |
const {width} = dockview.floatingGroups[index].group |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
@@ -163,7 +164,7 @@ | |||
if (index > -1) { | |||
this._listeners.splice(index, 1); | |||
} | |||
else if (Emitter.ENABLE_TRACKING); | |||
else if (Emitter.ENABLE_TRACKING) ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces
)
else if (Emitter.ENABLE_TRACKING) ; | |
else if (Emitter.ENABLE_TRACKING) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
}); | ||
} | ||
|
||
const autoHide = group => { | ||
const dockview = group.api.accessor; | ||
const type = group.model.location.type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring
)
const type = group.model.location.type | |
const {type} = group.model.location |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
dock(group, 'drawer') | ||
} | ||
if(type == 'grid'){ | ||
if (!canFloat(group)) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces
)
if (!canFloat(group)) return; | |
if (!canFloat(group)) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
||
btn.addEventListener('click', () => { | ||
const fgWrapper = floatingGroup.element.parentElement | ||
const activePanel = floatingGroup.activePanel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring
)
const activePanel = floatingGroup.activePanel | |
const {activePanel} = floatingGroup |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
btn.addEventListener('click', () => { | ||
const fgWrapper = floatingGroup.element.parentElement | ||
const activePanel = floatingGroup.activePanel | ||
const parentElement = activePanel.view.content.element.parentElement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring
)
const parentElement = activePanel.view.content.element.parentElement | |
const {parentElement} = activePanel.view.content.element |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
const title = group.activePanel?.title || group.panels[0]?.title | ||
const groupId = group.api.accessor.id + '_' + group.id | ||
const btnEle = group.api.accessor.element.parentElement.parentElement.querySelector(`.bb-dockview-btn-wrapper>[groupId="${groupId}"]`) | ||
if(!btnEle) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces
)
if(!btnEle) return | |
if (!btnEle) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
add ShowPin parameter
Summary of the changes (Less than 80 chars)
Description
fixes #154
Customer Impact
Regression?
[If yes, specify the version the behavior has regressed from]
Risk
[Justify the selection above]
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a 'ShowPin' parameter to the DockView component to allow control over the visibility of pin icons. Enhance the component to support floating groups with customizable positions and dimensions, and introduce drawer handles for better management of floating groups.
New Features:
Enhancements: