Skip to content
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

Add support for "fluid" drawing #135

Closed
jonhoo opened this issue Jul 8, 2019 · 2 comments · Fixed by #136
Closed

Add support for "fluid" drawing #135

jonhoo opened this issue Jul 8, 2019 · 2 comments · Fixed by #136
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@jonhoo
Copy link
Owner

jonhoo commented Jul 8, 2019

Currently, the width of the flamegraph, and the individual sample boxes, is set in pixels. It'd be really cool if we could instead use percentages, as it would mean that users with a wider monitor would automatically be able to take advantage of their screen size.

@jonhoo jonhoo added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Jul 8, 2019
@jasonrhansen
Copy link
Collaborator

Since we have an option to set the image width in pixels, we need to decide how this feature will affect that.

Should we keep the default width set to 1200 pixels to match upstream, or would you rather have 100% as the default?

Do we want to enhance the width option to allow percentages in addition to pixels? So --width=1200 means 1200 pixels, but --width=60% means 60%?

@jonhoo
Copy link
Owner Author

jonhoo commented Jul 8, 2019

I think the trick here will be something along the lines of:

  • Set all "internal" widths in percent (i.e., everything except the canvas)
  • If the user sets a width, use that as the width of the canvas.
  • Otherwise, don't set a width for the canvas, which I believe will cause it to be "whatever width is available".

You're totally right that this'll break with upstream default behavior, but I'm okay with that in this instance.

We may have to fiddle with the zoom code in javascript too to get the zoom to work correctly. Alternatively, I'd like us to move the zoom to use viewbox instead, but that may be a bigger task. This article may help in figuring out how to set things correctly. We're in a bit of a special case since we mostly don't care about aspect ratio, but I think we could figure something out.

I don't think we should allow percentage widths as arguments. I don't really see a use for it. I don't even really see a use for --width except when combined with height-related attributes to try to get a particular aspect ratio. I'm almost inclined to deprecate --width (it's an SVG after all!), but that might be taking it too far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants