fix: DH-18685: Remove top margin from chart#1126
fix: DH-18685: Remove top margin from chart#1126jnumainville merged 20 commits intodeephaven:mainfrom
Conversation
jnumainville
requested review from
a team,
Copilot and
mattrunyon
and removed request for
a team
There was a problem hiding this comment.
PR Overview
This PR addresses the removal of the default 60-pixel top margin added by Plotly Express when a title is not specified. Key changes include updating the figure layout in generate.py to remove the top margin and modifying numerous test cases to reflect the removal of the margin.
Reviewed Changes
| File | Description |
|---|---|
| plugins/plotly-express/src/deephaven/plot/express/deephaven_figure/generate.py | Updates the layout update call to remove the default top margin from the figure. |
| plugins/plotly-express/test/deephaven/plot/express/plots/*.py | Adjusts expected layouts in tests by removing the top margin value. |
Copilot reviewed 45 out of 45 changed files in this pull request and generated no comments.
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
mattrunyon
left a comment
mattrunyon
left a comment
There was a problem hiding this comment.
Do we have e2e tests with titles? Wanted to make sure this doesn't break things with titles, but haven't run the code yet
| # px adds a margin of 60 if a title is not specified | ||
| # since most charts still use px at their core and | ||
| # this isn't user controlled in any way, remove it after | ||
| # the figure is already created | ||
| px_fig.update_layout( | ||
| margin_t=None, | ||
| ) |
There was a problem hiding this comment.
Just to make sure, the user couldn't set margin before this change and they still can't? Or the user can through unsafe_update_figure?
I'm guessing if we wanted to expose this as a prop in the future we would need to revisit this a bit to check if the user set the margin or we did.
Is there any way to set this via default themeing in plotly or is this added separately from the theme by plotly?
There was a problem hiding this comment.
We don't have any e2e tests currently for title. The indicator review has one but probably a good idea to add more so I can do that. Titles did work correctly as per the example though. px has some conditional logic that adds this top margin only if title is not specified and that is handled by px still for most plots.
They could and still can set it through unsafe_update_figure. That is applied after a few returns outside of this.
If we wanted to expose it as a prop we could just remove this and have a default top margin of None. Then it would be added to the props list at the top of this file. I could do that anyways if we did think it was likely that we wanted to add that prop anytime soon.
This is added by plotly so we need to remove it anyways. I think we could theme this if we wanted to some day as well.
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
mattrunyon
left a comment
mattrunyon
left a comment
There was a problem hiding this comment.
I think there is an issue with the git history on this branch or you committed something to this branch that was meant for another
| expect(mockSubscribe).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it('should swap replaceable WebGL traces without blocker events if WebGL is not supported', async () => { |
There was a problem hiding this comment.
Is this test supposed to be on another branch? Seems unrelated to this PR
There was a problem hiding this comment.
Ah my bad I meant to comment about that. I added the scatter e2e test but that was broken because webgl doesn't work in firefox headless mode. So all this seemingly unrelated code is to fix that (as well as the general case when webgl doesn't work).
| setWebGlTraceType( | ||
| this.plotlyData, | ||
| webgl && this.isWebGlSupported, | ||
| this.webGlTraceIndices | ||
| ); |
There was a problem hiding this comment.
Same question about why there's webgl stuff in this PR
| * | ||
| * @returns True if Web GL is supported, false otherwise | ||
| */ | ||
| export function isWebGLSupported(): boolean { |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
I am pulling the webgl stuff into a different review that will need to be merged first |
Headless firefox mode doesn't work properly with webgl, so we need to detect if webgl is allowed. This was causing issues for #1126
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
1 similar comment
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
Plotly Express adds a top margin that I have now removed. Fixes DH-18685