Skip to content

fix: donut center minimum font size #1053

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 17, 2021
Merged

fix: donut center minimum font size #1053

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
271db6d
fix: donut center minimum font size
alok-traceable Aug 9, 2021
922eaac
Merge branch 'main' into minimum_font_size_on_donut
alok-traceable Aug 10, 2021
8237088
Merge branch 'main' into minimum_font_size_on_donut
alok-traceable Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion projects/observability/src/shared/components/donut/donut-builder.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export class DonutBuilderService extends D3VisualizationBuilderService<
private static readonly DONUT_ARC_CLASS: string = 'donut-arc';

private static readonly DONUT_PADDING_PX: number = 10;
private static readonly MIN_FONT_SIZE_FOR_TITLE: number = 12;
private static readonly MAX_FONT_SIZE_FOR_TITLE: number = 15;
private static readonly MAX_FONT_SIZE_FOR_VALUE: number = 64;

Expand Down Expand Up @@ -74,7 +75,10 @@ export class DonutBuilderService extends D3VisualizationBuilderService<
.attr('transform', `translate(0,-${dimensions.donutInnerRadius / 2})`)
.attr(
'font-size',
Math.min(Math.floor(dimensions.donutInnerRadius / 8), DonutBuilderService.MAX_FONT_SIZE_FOR_TITLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Code wise looks fine to me.
It is just we have to see what could be the min size for donut and for that case does this title text overflow?
cc: @jake-bassett @anandtiwary

Copy link
Contributor

Choose a reason for hiding this comment

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

fair point. Is there a min size for the donut too? If yes, then deciding a min font size would be easier.

Copy link
Contributor

@jake-bassett jake-bassett Aug 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We don't necessarily have a known minimum size for the donut itself, but for our screen sizes we try to test down to 1366-by-768.

As a matter of what to optimize for, I would be more concerned about the text spilling out than it getting too small. I think the overflow would look more broken than the text being illegible.

Copy link
Contributor Author

@alok-traceable alok-traceable Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So, there would be 2 aspects of this,

  1. Donut size, with smaller donut size than this, ideally we shouldn't put anything in between. If it is required then we can just put "value with a tooltip" and the font size of the value is still calculated so this PR doesn't affect that logic.
  2. Font size of the title, the most important thing here should be whether the feature we are putting is usable or not, with a smaller (calculated) font size, even though it looks good but it can't be read without a lens. The title is always static if it is overflowing we should be changing the donut size to make it useable and not reduce the font size to make it look good. IMO usability is always prefered over visuals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on both points. Pick a reasonable cutoff size to hide the center value all together. Maybe if the font gets smaller than 16px or so. You could play with it and see if keeping some indicator there is a value there makes sense. It could be just an ellipsis with a tooltip or the like. Use your best judgement and we can iterate on it a bit if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I won't recommend hiding the center content dynamically, this would be adding a factor to unpredictability also this would take away the freedom of putting only value at the center without a title.
Anyway, it is a static title and while developing we can always make sure it is not overflowing in any of the supported screen widths and given static title length.

Math.min(
Math.max(DonutBuilderService.MIN_FONT_SIZE_FOR_TITLE, Math.floor(dimensions.donutInnerRadius / 8)),
DonutBuilderService.MAX_FONT_SIZE_FOR_TITLE
)
);

visualizationContainer
Expand Down