Skip to content

Copy width/height calculating logic from kaleido - #5325

Merged
emilykl merged 10 commits into
plotly:mainfrom
geopozo:andrew/better-kaleido-defaults
Aug 19, 2025
Merged

Copy width/height calculating logic from kaleido#5325
emilykl merged 10 commits into
plotly:mainfrom
geopozo:andrew/better-kaleido-defaults

Conversation

@ayjayt

@ayjaytayjayt commented Aug 13, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the code generator and not the generated files.
  • I have added tests or modified existing tests.
  • For a new feature, I have added documentation examples (please see the doc checklist as well).
  • I have added a CHANGELOG entry if changing anything substantial.
  • For a new feature or a change in behavior, I have updated the relevant docstrings in the code.

plotly/Kaleido#378

@ayjaytayjayt mentioned this pull request Aug 13, 2025
Open
@ayjayt

ayjayt commented Aug 13, 2025•
edited by emilykl
Loading

Copy link
Copy Markdown
ContributorAuthor

closes plotly/Kaleido#378

emilykl
emilykl reviewed Aug 14, 2025
View reviewed changes
Comment threadplotly/io/_kaleido.py Outdated
emilykl
emilykl reviewed Aug 14, 2025
View reviewed changes
Comment threadplotly/io/_kaleido.py Outdated
@emilykl

emilykl commented Aug 14, 2025

Copy link
Copy Markdown
Contributor

Looks good @ayjayt ! Just run ruff format on plotly/io/_kaleido.py so the CI will pass.

Let's add a test for this one. I'm happy to do that if you don't have the bandwidth, just let me know.

ayjaytand others added 6 commits August 14, 2025 15:00
@ayjayt@emilykl
Add proper defaults to dict .get()
e8049a2
Co-authored-by: Emily KL <4672118+emilykl@users.noreply.github.com>
@ayjayt@emilykl
Add proper defaults to dict .get() x2
8036084
Co-authored-by: Emily KL <4672118+emilykl@users.noreply.github.com>
@ayjayt
Format with ruff.
bcdccf4
@ayjayt
Add some width/height testing.
f1e7607
@ayjayt
Fix test formatting.
1b962ec
@ayjayt
Fix variable access.
20035c6
@ayjayt
ayjayt marked this pull request as ready for review August 14, 2025 21:04
@ayjayt
ayjayt requested a review from emilyklAugust 14, 2025 21:04
emilykl
emilykl reviewed Aug 15, 2025
View reviewed changes
Comment threadtests/test_optional/test_kaleido/test_kaleido.py Outdated
mock_get_chrome.assert_called_once()


def create_figure(width=None, height=None):

@emilyklemilyklAug 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you move these two utility functions to the top of the file so they're not mixed in with the test functions?

emilykl
emilykl reviewed Aug 15, 2025
View reviewed changes
Comment threadtests/test_optional/test_kaleido/test_kaleido.py Outdated
fig = create_figure()
svg_bytes = pio.to_image(fig, format="svg")
width, height = parse_svg_dimensions(svg_bytes)
assert width is not None and height is not None, (

@emilyklemilyklAug 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to import pio.defaults here and check that the dimensions equal the defaults

emilykl
emilykl approved these changes Aug 15, 2025
View reviewed changes

@emilyklemilykl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ayjayt ! 🚀

@ayjayt
ayjayt requested a review from emilyklAugust 18, 2025 20:28
@emilykl

emilykl commented Aug 19, 2025

Copy link
Copy Markdown
Contributor

@ayjayt Looks good to me -- may I merge?

@emilykl
emilykl merged commit 7639653 into plotly:mainAug 19, 2025
8 checks passed
@ayjayt
ayjayt deleted the andrew/better-kaleido-defaults branch August 19, 2025 18:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emilyklemilyklAwaiting requested review from emilykl

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ayjayt@emilykl