Skip to content

Conversation

alok-traceable
Copy link
Contributor

@alok-traceable alok-traceable commented Aug 9, 2021

Description

Change to have a minimum font size of the title text at the donut center, else it can go to unreadable font sizes.

Testing

Tested locally not breaking anything.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@alok-traceable alok-traceable requested a review from a team as a code owner August 9, 2021 04:44
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #1053 (8237088) into main (523afbc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1053   +/-   ##
=======================================
  Coverage   85.07%   85.08%           
=======================================
  Files         822      822           
  Lines       16923    16924    +1     
  Branches     2045     2045           
=======================================
+ Hits        14398    14399    +1     
  Misses       2494     2494           
  Partials       31       31           
Impacted Files Coverage Δ
...c/shared/components/donut/donut-builder.service.ts 85.71% <100.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 523afbc...8237088. Read the comment docs.

@github-actions

This comment has been minimized.

@@ -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

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

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.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@alok-traceable alok-traceable merged commit f8d8b7c into main Aug 17, 2021
@alok-traceable alok-traceable deleted the minimum_font_size_on_donut branch August 17, 2021 07:51
@github-actions
Copy link

Unit Test Results

    4 files  263 suites   15m 17s ⏱️
947 tests 947 ✔️ 0 💤 0 ❌
954 runs  954 ✔️ 0 💤 0 ❌

Results for commit f8d8b7c.

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 this pull request may close these issues.

4 participants