-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
base: master
Are you sure you want to change the base?
Conversation
We will need to configure different bubble styles when on different charts. This PR makes the padding configurable if needed. It also removes left padding from first bubble and right padding from last padding.
bfb5f1d
to
0241175
Compare
<BarChart>
// bubbleStartX bubbleEndX | ||
// | | | ||
// v v | ||
// ---------------- ---------------- | ||
// | | | | | ||
// ---------------- ---------------- | ||
// ^ ^ | ||
// |--| | ||
// bubblePadding.x |
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.
note my artisanal ascii art
@@ -1,10 +1,8 @@ | |||
export const BUBBLE_SERIES_ID = '__release_bubble__'; | |||
export const BUBBLE_AREA_SERIES_ID = '__release_bubble_area__'; | |||
export const DEFAULT_BUBBLE_SIZE = 12; | |||
export const DEFAULT_BUBBLE_SIZE = 4; |
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.
Previously, the bubble size was the entire height including whitespace around it. This was because I started w/ the chart configuration and calculated everything else based on that.
I inverted this so that we define the actual bubble pixel height and calculate the chart offsets based on that + padding.
|
||
// "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; |
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.
calculate this since padding is now configurable.
We may need to configure different bubble styles when on different charts. This PR makes the padding configurable if needed. It also removes left padding from first bubble and right padding from last padding and fixes the vertical alignment of the bubble as well.
ref #85775