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

Consider using text-anchor for right-aligned strings #247

Closed
nagisa opened this issue Jun 16, 2022 · 2 comments · Fixed by #254
Closed

Consider using text-anchor for right-aligned strings #247

nagisa opened this issue Jun 16, 2022 · 2 comments · Fixed by #254

Comments

@nagisa
Copy link

nagisa commented Jun 16, 2022

I’ve used a somewhat modified version of the inferno crate to construct flamegraphs for a blog post of mine. I have made a couple of changes that I think some contributor here may be interested in implementing properly in the project proper.

The flamegraph has two strings that are expected to be aligned to the right side of the image: the “Search”/“Reset search” at the top and “Matched: 42%” at the bottom. Right now they are aligned by giving the 100px of “space” from the side, which can fail to fit the string if the font size is increased or a wider font is used.

Instead consider using a text-anchor: end and placing the text node at a constant location where the strings should end:

diff --git a/src/flamegraph/mod.rs b/src/flamegraph/mod.rs
index 4ba2290..a90e742 100644
--- a/src/flamegraph/mod.rs
+++ b/src/flamegraph/mod.rs
@@ -228,8 +199,8 @@ text {{ font-family:{}; font-size:{}px; fill:rgb(0,0,0); }}
         svg,
         &mut buf,
         TextItem {
-            x: Dimension::Pixels(image_width as usize - super::XPAD - 100),
-            y: (opt.font_size * 2) as f64,
+            x: Dimension::Pixels(image_width as usize - super::XPAD),
+            y: (opt.font_size + 5) as f64,
             text: "Search".into(),
             extra: vec![("id", "search")],
         },
@@ -239,7 +210,7 @@ text {{ font-family:{}; font-size:{}px; fill:rgb(0,0,0); }}
         svg,
         &mut buf,
         TextItem {
-            x: Dimension::Pixels(image_width as usize - super::XPAD - 100),
+            x: Dimension::Pixels(image_width as usize - super::XPAD),
             y: (style_options.imageheight - (opt.ypad2() / 2)) as f64,
             text: " ".into(),
             extra: iter::once(("id", "matched")),
diff --git a/src/flamegraph/flamegraph.css b/src/flamegraph/flamegraph.css
index 0bc4184..d47a8b9 100644
--- a/src/flamegraph/flamegraph.css
+++ b/src/flamegraph/flamegraph.css
@@ -1,7 +1,13 @@
+#matched { text-anchor:end; }
+#search { text-anchor:end; }
diff --git a/src/flamegraph/flamegraph.js b/src/flamegraph/flamegraph.js
index cf53d99..fae5f55 100644
--- a/src/flamegraph/flamegraph.js
+++ b/src/flamegraph/flamegraph.js
@@ -40,8 +40,8 @@ function init(evt) {
 
             // Keep search elements at a fixed distance from right edge.
             var svgWidth = svg.width.baseVal.value;
-            searchbtn.attributes.x.value = svgWidth - xpad - 100;
-            matchedtxt.attributes.x.value = svgWidth - xpad - 100;
+            searchbtn.attributes.x.value = svgWidth - xpad;
+            matchedtxt.attributes.x.value = svgWidth - xpad;
         };

I have used CSS here to specify this attribute, but specifying it as a proper SVG attribute may make more sense if these flamegraphs are intended to work without CSS present.

@jonhoo
Copy link
Owner

jonhoo commented Jun 19, 2022

I agree, that seems like a good change! Any takers? I think CSS should be fine here, although we do need to make sure that SVG supports all the relevant CSS constructs (I don't know that SVGs have "full" CSS support).

@nagisa
Copy link
Author

nagisa commented Jun 19, 2022

text-anchor is a native SVG attribute so it should be supported quite widely. In addition to that, the positioning of the text strings at the top of a flamegraph could be further simplified with the use of dominant-baseline="hanging".

Urgau added a commit to Urgau/inferno that referenced this issue Jul 17, 2022
This was proposed by `@nagisa` in
jonhoo#247, I just applied the
suggestions, tested that the result was as expected.

I tried the suggestion of `dominant-baseline="hanging"`, but it will
need further work to make it usefull as applying instead will just
increase the size of the css without reducing enough in the SVG.

Co-authored-by: Simonas Kazlauskas <git@kazlauskas.me>
Fixes jonhoo#247
Urgau added a commit to Urgau/inferno that referenced this issue Jul 26, 2022
This was proposed by `@nagisa` in
jonhoo#247, I just applied the
suggestions, tested that the result was as expected.

I tried the suggestion of `dominant-baseline="hanging"`, but it will
need further work to make it usefull as applying instead will just
increase the size of the css without reducing enough in the SVG.

Co-authored-by: Simonas Kazlauskas <git@kazlauskas.me>
Fixes jonhoo#247
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants