-
-
Notifications
You must be signed in to change notification settings - Fork 1
Pass image scale info on treemap element data model if available #524
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
| def to_dict(self) -> Dict[str, Any]: | ||
| """Convert to dictionary with serializable datetime.""" | ||
| data = self.model_dump() | ||
| data = self.model_dump(exclude_none=True) |
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.
@rbro112 this is the fix to not have path: null or misc: null included in the treemap data when passed to the frontend
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.
Awesome, I think we might need support in the backend/frontend to ensure we parse these all properly, can you just do an E2E test to make sure downstream size processing related code either responds gracefully to missing fields or uncover any potential errors?
|
|
||
| export interface TreemapElement { | ||
| /** Display name of the element */ | ||
| name: string; |
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.
Might need to separate out these FE changes first as the deploys could get out of sync, especially if we need to make more fields optional
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 in this case we're OK since the only stuff we're adding is optional on both ends, so even if one of them deploys before the other, it shouldn't matter
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.
(oh and fwiw this is in launchpad, but i know what you mean)
rbro112
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.
Looks good, might need to split FE changes out first to ensure we accommodate optional fields (if more fields need to be made optional)
…104879) Closes https://linear.app/getsentry/issue/EME-665/include-scale-parameter-in-the-output-for-images-in-treemap <img width="762" height="493" alt="Screenshot 2025-12-12 at 11 45 28 AM" src="https://github.com/user-attachments/assets/6f6e748d-0757-455d-a073-f6b2b801fbc1" /> Pre-req PRs: - getsentry/sentry-cli#3029 - getsentry/launchpad#524
Prerequisite for https://linear.app/getsentry/issue/EME-665/include-scale-parameter-in-the-output-for-images-in-treemap
related CLI change: getsentry/sentry-cli#3029