-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(dashboards): Auto-size Big Number widget #76209
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
Conversation
Reset the known dimensions (both of them) and the known bounds, since the resize makes them irrelevant.
- place Tooltip lower in the hierarchy to prevent layout issues - increase the minimum font size
narsaynorath
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment around the error-ish condition where we reach the maximum depth. I think we should be tracking something here so we can look back and see if there are issues, otherwise it's failing silently. This looks great, I owe you a hot chocolate 😁
edwardgou-sentry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty cool solution! Thanks George!
|
@edwardgou-sentry @narsaynorath @JonasBa thanks for the reviews! I made some major changes to the component, so I'm re-requesting the review and marking comments as stale, since so much has changed! @narsaynorath I added instrumentation! I was going to, I swear, you caught me being lazy. The condition you asked about doesn't exist anymore, so I'm just logging the iteration count @JonasBa I've addressed, I think a lot of your concerns, thank you for those suggestions:
I did keep some of the old stuff:
The overall approach is still the same. I also updated the docs, while I was at it, and added more inline code comments. Thank you for your reviews, please look again when you have a chance 🙏🏻 |
|
|
||
| interface Props { | ||
| children: React.ReactNode; | ||
| maximumDifference?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were to use a component like this, this isnt something I'd want to have to think about. I think we should remove it, and revisit the component design if this ever becomes a requirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I guess that makes sense but you don't have to think about it, it's optional! I immediately thought that I'd want to change this if I use this in a component where fitment is important (e.g., in a headline or some other important visual element)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you go about guiding folks to set a good value here? What I'm afraid of is that everyone would just set it to 0 because they'd want it to be the exact size (which is kind of the goal already?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's okay, actually! In fact, maybe what I should do is set the default to 0 (or 1px just to be safe) and set it to 5px just in Dashboards where there might be many of these numbers and I don't wanna halt the UI thread too long. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you evaluate if that is a good config? Does that mean we'd need to care about how many big numbers we render and update this over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 okay let me see how much the iteration count actually affects the render time on pages with a lot of big numbers. If it's low, I will set the tolerance to 1px and remove the prop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! Sold! Removed it, and added a note about iteration counts and performance related to the threshold 👍🏻
Co-authored-by: Jonas <jonas.badalic@sentry.io>
| // Run the resize iteration in a loop. This blocks the main UI thread and prevents | ||
| // visible layout jitter. If this was done through a `ResizeObserver` or React State | ||
| // each step in the resize iteration would be visible to the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏅 for comments like these
Co-authored-by: Jonas <jonas.badalic@sentry.io>
|
Nice work @gggritso, I think you found a cleaner implementation that is less opinionated than before with the code being easier to follow! |
| childDimensions.height < parentDimensions.height | ||
| ) { | ||
| // The element is smaller than the parent, scale up | ||
| newFontSize = (fontSizeUpperBound.current + fontSize.current) / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on trimming or rounding the floating point values here? I dont think this is an issue tbh, but it might generate some really long floats here :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could! Ostensibly modern fonts handle sub-pixel rendering gracefully enough. Are you worried about DOM bloat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the browsers will handle it fine (though different platforms render text quite differently). I think its more likely just going to be an instance where someone inevitably sees a high precision float in the DOM and starts asking around or making up some wrong ideas, which is a conversation you can spare yourself from having :D
JonasBa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work, A+ for implementation, stories and code comments!
Before:

After:

Bonus:

Explanation
The "Big Number" widget is a Dashboards feature where a user can make a query like
count()and get a big honkin' number that shows how many transactions they have. The widget has some fancy CSS to help size it big (but not too big), but CSS alone isn't enough. In many cases, the big number gets truncated! That's not very nice or very useful.This PR changes the approach. Instead of CSS, it adds a new utility component called
AutoSizedText. This component mimics the height and width of its parent, and then iteratively updates the font size of its contents until they fit nicely inside the parent. e.g.,Other Methods Considered
<text>elements. I didn't like this approach because font scaling by percentage is not at all the same as changing the font size. Modern typefaces do a lot of work to look legible at different sizes at the font level, and the only way to respect this is to set the correct font sizetransformhas the same problem as SVG scalingmeasureTextto get the dimensions, but that's fairly complicatedFAQs
I think the most obvious question is what's the performance like. In short, it's not great, but I did what I could. The most important thing is wrapping the updates in
useTransitionwhich will mark them as lower priority. This will allow other, higher-priority renders (like interactions with other dropdowns) to interrupt the resizing logic. I also limited the iteration count to 5, and debounced the resize listener. This seems to work well enough even with CPU throttling set pretty high.Overall I'm not too worried because resizing is not a common operation, and it's still pretty fast compared to how long a Dashboard resize already takes (a long time).
The second is testing. Short of literal snapshots (which I don't think we do anymore) I couldn't come up with anything that felt more worthwhile than manual QA. You tell me, though.
Fixes #75730