Add option for fit to screen #721
Conversation
There was a problem hiding this comment.
this looks good to me but I wonder if we should have it inverted (--fit-to-screen / --fit) to enable as mentioned in #659. related discussion #601 (comment)
I also wonder if we could do the fitting with just css instead of js
|
I think the '--fit-to-screen' option is better, too. |
|
Since the default behavior no longer adds the js, the test svgs need to be updated with |
|
hold off on this for a sec, need to think about this whole fit to screen thing |
|
now that the resizing is done with CSS, i wonder if anyone would still want to turn this off. i'll leave this up for a little longer to wait and see, but i suspect that #725 solves all resizing woes. |
Co-authored-by: gavin-ts <85081687+gavin-ts@users.noreply.github.com>
bluemir
force-pushed
the
add-option-disable-fit-to-screen
branch
from
0c4de64
to
a9da41d
Compare
d2renderers/d2svg/d2svg.go
Outdated
| if !opts.FitScreen { | ||
| fitScreenWrapper = "" | ||
| } |
There was a problem hiding this comment.
I think this should be the same as setDimensions above now. setting dimensions disables the fit to screen behavior.
There was a problem hiding this comment.
I think we also want to rename the option to --no-fit or --sized since we want the fit behavior as default now
d2cli/main.go
Outdated
| SetDimensions: toPNG, | ||
| FitScreen: fitScreen, | ||
| NoFit: noFit, |
There was a problem hiding this comment.
Since NoFit does the same thing as SetDimensions, I think it is better to just have SetDimensions: toPNG || noFit, without another RenderOpt
d2renderers/d2svg/d2svg.go
Outdated
| if setDimensions && !opts.NoFit { | ||
| dimensions = fmt.Sprintf(` width="%d" height="%d"`, w, h) | ||
| } |
There was a problem hiding this comment.
I experimented with applying this change in a diagram and learned that it does not do what I expected. Please see the note here: #1080 (comment)
Add CLI option for disable
fit to screenscript. (--disable-fit-to-screen)It might be solution for this issue #659 (comment)
Test failed when I run
./ci/test.shin my laptop, but it looks like not related to this change.(I have no idea to fix it)