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

feat(ui): Make release bubble padding configurable #86841

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,10 +1,2 @@
export const BUBBLE_SERIES_ID = '__release_bubble__';
export const BUBBLE_AREA_SERIES_ID = '__release_bubble_area__';
export const DEFAULT_BUBBLE_SIZE = 12;

// "padding" around the bubbles as it is drawn on canvas vs CSS, this may need
// to move into `renderReleaseBubble` if it needs to be customizable
export const RELEASE_BUBBLE_Y_PADDING = 8;
export const RELEASE_BUBBLE_Y_HALF_PADDING = RELEASE_BUBBLE_Y_PADDING / 2;
Copy link
Member Author

Choose a reason for hiding this comment

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

calculate this since padding is now configurable.

export const RELEASE_BUBBLE_X_PADDING = 1;
export const RELEASE_BUBBLE_X_HALF_PADDING = RELEASE_BUBBLE_X_PADDING / 2;
Comment on lines -3 to -10
Copy link
Member Author

Choose a reason for hiding this comment

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

moved these as default args in the hook

Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,6 @@ import {useUser} from 'sentry/utils/useUser';
import {
BUBBLE_AREA_SERIES_ID,
BUBBLE_SERIES_ID,
DEFAULT_BUBBLE_SIZE,
RELEASE_BUBBLE_X_HALF_PADDING,
RELEASE_BUBBLE_X_PADDING,
RELEASE_BUBBLE_Y_HALF_PADDING,
RELEASE_BUBBLE_Y_PADDING,
} from 'sentry/views/dashboards/widgets/timeSeriesWidget/releaseBubbles/constants';
import {createReleaseBubbleHighlighter} from 'sentry/views/dashboards/widgets/timeSeriesWidget/releaseBubbles/createReleaseBubbleHighlighter';
import type {Bucket} from 'sentry/views/dashboards/widgets/timeSeriesWidget/releaseBubbles/types';
Expand Down Expand Up @@ -147,6 +142,7 @@ function createReleaseBubbleMouseListeners({
}

interface ReleaseBubbleSeriesProps {
bubblePadding: {x: number; y: number};
bubbleSize: number;
buckets: Bucket[];
chartRef: React.RefObject<ReactEchartsRef>;
Expand All @@ -165,6 +161,7 @@ function ReleaseBubbleSeries({
chartRef,
theme,
bubbleSize,
bubblePadding,
dateFormatOptions,
}: ReleaseBubbleSeriesProps): CustomSeriesOption | null {
const totalReleases = buckets.reduce((acc, {releases}) => acc + releases.length, 0);
Expand Down Expand Up @@ -192,10 +189,10 @@ function ReleaseBubbleSeries({
* Renders release bubbles underneath the main chart
*/
const renderReleaseBubble: CustomSeriesRenderItem = (
_params: CustomSeriesRenderItemParams,
params: CustomSeriesRenderItemParams,
api: CustomSeriesRenderItemAPI
) => {
const dataItem = data[_params.dataIndex];
const dataItem = data[params.dataIndex];

if (!dataItem) {
return null;
Expand All @@ -221,14 +218,46 @@ function ReleaseBubbleSeries({
// Width between two timestamps for timeSeries
const width = bubbleEndX - bubbleStartX;

//
const shape = {
x: bubbleStartX + RELEASE_BUBBLE_X_HALF_PADDING,
y: bubbleStartY + RELEASE_BUBBLE_Y_HALF_PADDING,
width: width - RELEASE_BUBBLE_X_PADDING,
// currently we have a static height, but this may need to change since
// we have charts of different dimensions
height: bubbleSize - RELEASE_BUBBLE_Y_PADDING,
// Padding is on both left/right sides to try to center the bubble
//
// bubbleStartX bubbleEndX
// | |
// v v
// ---------------- ----------------
// | | | |
// ---------------- ----------------
// ^ ^
// |--|
// bubblePadding.x

// No left-padding on first bubble
x: bubbleStartX + (params.dataIndex === 0 ? 0 : bubblePadding.x),

// No right-padding on last bubble
// We don't decrease width on first bubble to make up for lack of
// left-padding
width:
width -
(params.dataIndex === 0 || params.dataIndex === data.length - 1
? 0
: bubblePadding.x),

// We configure base chart's grid and xAxis to create a gap size of
// `bubbleSize`. We then have to configure `y` and `height` to fit within this
//
// ----------------- grid bottom
// | bubblePadding.y
// | bubbleSize
// | bubblePadding.y
// ----------------- = xAxis offset

// idk exactly what's happening but we need a 1 pixel buffer to make it
// properly centered. I want to guess because we are drawing below the
// xAxis, and we have to account for the pixel being drawn in the other
// direction. You can see this if you set the x-axis offset to 0 and compare.
y: bubbleStartY + bubblePadding.y + 1,
height: bubbleSize,

// border radius
r: 0,
Expand Down Expand Up @@ -294,6 +323,7 @@ ${t('Click to expand')}

interface UseReleaseBubblesParams {
chartRef: React.RefObject<ReactEchartsRef>;
bubblePadding?: {x: number; y: number};
bubbleSize?: number;
chartRenderer?: (rendererProps: {
end: Date;
Expand All @@ -310,7 +340,8 @@ export function useReleaseBubbles({
releases,
minTime,
maxTime,
bubbleSize = DEFAULT_BUBBLE_SIZE,
bubbleSize = 4,
bubblePadding = {x: 2, y: 1},
}: UseReleaseBubblesParams) {
const organization = useOrganization();
const {openDrawer} = useDrawer();
Expand All @@ -335,6 +366,9 @@ export function useReleaseBubbles({
};
}

// Read comments in `renderReleaseBubble()` regarding the +1
const totalBubblePaddingY = 1 + bubblePadding.y * 2;

return {
createReleaseBubbleHighlighter,

Expand All @@ -354,6 +388,7 @@ export function useReleaseBubbles({
releaseBubbleSeries: ReleaseBubbleSeries({
buckets,
bubbleSize,
bubblePadding,
chartRef,
theme,
releases,
Expand All @@ -369,7 +404,7 @@ export function useReleaseBubbles({
// configure `axisLine` and `offset` to move axis line below 0 so that
// bubbles sit between bottom of the main chart and the axis line
axisLine: {onZero: false},
offset: bubbleSize,
offset: bubbleSize + totalBubblePaddingY,
},

/**
Expand All @@ -378,7 +413,7 @@ export function useReleaseBubbles({
releaseBubbleGrid: {
// Moves bottom of grid "up" `bubbleSize` pixels so that bubbles are
// drawn below grid (but above x axis label)
bottom: bubbleSize,
bottom: bubbleSize + totalBubblePaddingY,
},
};
}
Loading