fix: remove whitespace and title overlapping#1330
Open
rajnisht7 wants to merge 22 commits into
Open
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts Plotly layout handling in PlotPane to reduce excess whitespace and prevent the plot title from overlapping the modbar by normalizing the layout, converting string titles to objects, and setting top margins and title vertical position based on title presence. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates PlotPane’s Plotly layout handling to normalize layout.title and adjust top margin based on title presence.
Changes:
- Introduces a local
layoutvariable derived fromcontent.layout. - Normalizes string titles into
{ text }objects and adjustslayout.margin.t/layout.title.y. - Keeps Plotly’s re-render mechanism via
datarevision.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
layoutvariable is only passed by reference tocontent.layoutwhencontent.layoutis already defined; if it's undefined you end up modifying a separate object and still pass the originalcontent.layouttoPlotly.react, so consider either initializingcontent.layoutup front or operating oncontent.layoutdirectly to avoid this discrepancy. - When setting
layout.margin.tand converting the title from string to object, you currently overwrite any existingmargin.tortitleobject configuration; it would be safer to only apply these defaults when the respective properties are not already set so that custom layouts are preserved.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `layout` variable is only passed by reference to `content.layout` when `content.layout` is already defined; if it's undefined you end up modifying a separate object and still pass the original `content.layout` to `Plotly.react`, so consider either initializing `content.layout` up front or operating on `content.layout` directly to avoid this discrepancy.
- When setting `layout.margin.t` and converting the title from string to object, you currently overwrite any existing `margin.t` or `title` object configuration; it would be safer to only apply these defaults when the respective properties are not already set so that custom layouts are preserved.
## Individual Comments
### Comment 1
<location path="js/panes/PlotPane.js" line_range="25-28" />
<code_context>
const maxsmoothvalue = 100;
const [smoothWidgetActive, setSmoothWidgetActive] = useState(false);
const [smoothvalue, setSmoothValue] = useState(1);
+ let layout = content.layout || {};
// private events
</code_context>
<issue_to_address>
**issue (bug_risk):** Layout normalization is done on a local variable but not used consistently when calling Plotly/react or setting datarevision.
Because `layout` is normalized and mutated while `Plotly.react` and `datarevision` still use `content.layout`, you can end up both throwing (when `content.layout` is falsy) and not applying the normalized title/margin to the rendered plot.
To fix this, make a single canonical layout object and use it everywhere, e.g. either:
```js
let layout = content.layout || {};
layout.datarevision = props.version;
// title / margin normalization on `layout`...
Plotly.react(contentID, data.concat(smooth_data), layout, { ... });
```
or:
```js
content.layout = content.layout || {};
const layout = content.layout;
layout.datarevision = props.version;
// title / margin normalization on `layout`...
Plotly.react(contentID, data.concat(smooth_data), content.layout, { ... });
```
</issue_to_address>
### Comment 2
<location path="js/panes/PlotPane.js" line_range="139-155" />
<code_context>
+ layout.title = { text: layout.title };
+ }
+
+ layout.margin = layout.margin || {};
+
+ if (layout.title.text) {
+ layout.margin.t = 65;
+ layout.title.y = 0.9;
+ } else {
+ layout.margin.t = 30;
+ }
+ } else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Title-based margin logic overwrites any existing top margin configuration.
This always sets `layout.margin.t` to `65` or `30`, overwriting any `margin.t` explicitly provided in `content.layout` (e.g., for legends, annotations, or long titles).
To preserve caller-defined margins while keeping the new defaults, only set `margin.t` when it’s unset:
```js
layout.margin = layout.margin || {};
if (layout.margin.t == null) {
layout.margin.t = layout.title && layout.title.text ? 65 : 30;
}
```
This lets explicit `margin.t` values take precedence.
```suggestion
layout.margin = layout.margin || {};
if (layout.title) {
if (typeof layout.title === 'string') {
layout.title = { text: layout.title };
}
if (layout.title.text) {
if (layout.margin.t == null) {
layout.margin.t = 65;
}
layout.title.y = 0.9;
} else if (layout.margin.t == null) {
layout.margin.t = 30;
}
} else if (layout.margin.t == null) {
layout.margin.t = 30;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+139
to
+155
| if (layout.title) { | ||
| if (typeof layout.title === 'string') { | ||
| layout.title = { text: layout.title }; | ||
| } | ||
|
|
||
| layout.margin = layout.margin || {}; | ||
|
|
||
| if (layout.title.text) { | ||
| layout.margin.t = 65; | ||
| layout.title.y = 0.9; | ||
| } else { | ||
| layout.margin.t = 30; | ||
| } | ||
| } else { | ||
| layout.margin = layout.margin || {}; | ||
| layout.margin.t = 30; | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Title-based margin logic overwrites any existing top margin configuration.
This always sets layout.margin.t to 65 or 30, overwriting any margin.t explicitly provided in content.layout (e.g., for legends, annotations, or long titles).
To preserve caller-defined margins while keeping the new defaults, only set margin.t when it’s unset:
layout.margin = layout.margin || {};
if (layout.margin.t == null) {
layout.margin.t = layout.title && layout.title.text ? 65 : 30;
}This lets explicit margin.t values take precedence.
Suggested change
| if (layout.title) { | |
| if (typeof layout.title === 'string') { | |
| layout.title = { text: layout.title }; | |
| } | |
| layout.margin = layout.margin || {}; | |
| if (layout.title.text) { | |
| layout.margin.t = 65; | |
| layout.title.y = 0.9; | |
| } else { | |
| layout.margin.t = 30; | |
| } | |
| } else { | |
| layout.margin = layout.margin || {}; | |
| layout.margin.t = 30; | |
| } | |
| layout.margin = layout.margin || {}; | |
| if (layout.title) { | |
| if (typeof layout.title === 'string') { | |
| layout.title = { text: layout.title }; | |
| } | |
| if (layout.title.text) { | |
| if (layout.margin.t == null) { | |
| layout.margin.t = 65; | |
| } | |
| layout.title.y = 0.9; | |
| } else if (layout.margin.t == null) { | |
| layout.margin.t = 30; | |
| } | |
| } else if (layout.margin.t == null) { | |
| layout.margin.t = 30; | |
| } |
tonypzy
reviewed
May 27, 2026
tonypzy
left a comment
Contributor
There was a problem hiding this comment.
Hard-coded values occurs in this PR. These values may need to be adjusted if the Plotly version is updated or the pane layout changes.
i suggest hiding modbar. When the mouse moves to the top of pane, modbar appears.
Let's see what others think of
Contributor
Author
|
@tonypzy as checked plotly expects numeric value in padding |
Jayantparashar10
suggested changes
May 31, 2026
Jayantparashar10
left a comment
Contributor
There was a problem hiding this comment.
content.layout = content.layout || {}
let layout = content.layout || {};
The || {} fallback on line 2 can never fire. Simplify to:
content.layout = content.layout || {};
const layout = content.layout;
Comment on lines
+25
to
+26
| content.layout = content.layout || {} | ||
| let layout = content.layout; |
Comment on lines
+148
to
+156
| layout.margin.t = 65; | ||
| layout.title.y = 0.9; | ||
| } else { | ||
| layout.margin.t = 30; | ||
| } | ||
| } else { | ||
| layout.margin = layout.margin || {}; | ||
| layout.margin.t = 30; | ||
| } |
Saksham-Sirohi
requested changes
Jun 9, 2026
Saksham-Sirohi
left a comment
Collaborator
There was a problem hiding this comment.
Please resolve conflicts and ensure the test pass
Comment on lines
+25
to
+26
| content.layout = content.layout || {}; | ||
| let layout = content.layout; |
Comment on lines
+193
to
+209
| if (layout.title) { | ||
| if (typeof layout.title === 'string') { | ||
| layout.title = { text: layout.title }; | ||
| } | ||
|
|
||
| layout.margin = layout.margin || {}; | ||
|
|
||
| if (layout.title.text) { | ||
| layout.margin.t = 65; | ||
| layout.title.y = 0.9; | ||
| } else { | ||
| layout.margin.t = 30; | ||
| } | ||
| } else { | ||
| layout.margin = layout.margin || {}; | ||
| layout.margin.t = 30; | ||
| } |
Comment on lines
+198
to
+209
| layout.margin = layout.margin || {}; | ||
|
|
||
| if (layout.title.text) { | ||
| layout.margin.t = 65; | ||
| layout.title.y = 0.9; | ||
| } else { | ||
| layout.margin.t = 30; | ||
| } | ||
| } else { | ||
| layout.margin = layout.margin || {}; | ||
| layout.margin.t = 30; | ||
| } |
Comment on lines
+25
to
+26
| content.layout = content.layout || {}; | ||
| let layout = content.layout; |
Comment on lines
+198
to
+209
| layout.margin = layout.margin || {}; | ||
|
|
||
| if (layout.title.text) { | ||
| layout.margin.t = 65; | ||
| layout.title.y = 0.9; | ||
| } else { | ||
| layout.margin.t = 30; | ||
| } | ||
| } else { | ||
| layout.margin = layout.margin || {}; | ||
| layout.margin.t = 30; | ||
| } |
Manik-Khajuria-5
suggested changes
Jun 9, 2026
Manik-Khajuria-5
left a comment
Member
There was a problem hiding this comment.
@rajnisht7 Can u please address co pilot reviews and also ci test are failing please go through it one more time
Comment on lines
+25
to
+26
| content.layout = content.layout || {}; | ||
| const layout = content.layout; |
Comment on lines
+198
to
+205
| layout.margin = layout.margin || {}; | ||
|
|
||
| if (layout.title.text) { | ||
| layout.margin.t = 65; | ||
| layout.title.y = 0.9; | ||
| } else { | ||
| layout.margin.t = 30; | ||
| } |
Contributor
|
please resolve copilot issues |
Comment on lines
+25
to
+26
| content.layout = content.layout || {}; | ||
| let layout = content.layout; |
Comment on lines
+198
to
+205
| layout.margin = layout.margin || {}; | ||
|
|
||
| if (layout.title.text) { | ||
| layout.margin.t = 65; | ||
| layout.title.y = 0.9; | ||
| } else { | ||
| layout.margin.t = 30; | ||
| } |
|
|
||
| layout.margin = layout.margin || {}; | ||
|
|
||
| if (layout.title.text) { |
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
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.
Description
currently there is enough whitespace in the panes between graph and the box and the title also overlaps with the modbar
this PR resolves this issue
Motivation and Context
as there is enough space it makes the visual the bad and overlapping makes difficult to identify
How Has This Been Tested?
executed the server
verified the changes
Screenshots:
Before:
After:
Types of changes
Checklist:
py/visdom/VERSIONaccording to Semantic VersioningSummary by Sourcery
Adjust plot layout configuration to improve spacing and title positioning in graph panes.
Bug Fixes: