Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Hey @noahonyejese, thanks for changing this! I think if we tweak the logic to use actual chartWidth (or chartWidth / 2 where applicable), the values should be correct without a need for additional calculations.
If they still are not, we'd need to make sure we pass correct line height, font size, etc to the function – here maybe there would be a way to connect this to MUI typography variants somehow, if the text elements are in fact MUI components 👍
| (isOverlapping | ||
| ? axisLabelFontSize * Math.ceil(overlapAmount) + TITLE_VPADDING | ||
| : axisLabelFontSize + TITLE_VPADDING) * | ||
| ? axisLabelHeight * Math.ceil(overlapAmount) + TITLE_VPADDING | ||
| : axisLabelHeight + TITLE_VPADDING) * | ||
| 2 + | ||
| TOP_MARGIN |
There was a problem hiding this comment.
If we add width as above (chartWidth / 2) to getTextHeight, I am pretty sure we don't need this logic :) The goal of getTextHeight was to get "correct" height, without a need to multiply or divide things anymore 👍
There was a problem hiding this comment.
@bprusinowski I will try to refactor it, I have an Idea...
There was a problem hiding this comment.
But with this we don't need to know the number of lines, as the text would naturally wrap inside the measurement container so we get correct height back, right?
There was a problem hiding this comment.
the text is within a foreignObject which needs the exact height of all lines combined to ensure correct amount of space for the span element.
There was a problem hiding this comment.
Yes, but here I was thinking about getTextHeight method - the flow would be:
- get width of the label (half of chart width, potentially with some padding),
- get text height, passing the width to the function, so that the text can naturally wrap inside the measurement container,
- set foreignObject height using the text height.
I don't see a reason to get number of lines of the text, did I misunderstand something? 🥹
There was a problem hiding this comment.
aha yeah, no I meant that yes the getTextHeight returns the wrong number, if its 4 lines it returns f.e 45 instead of f.e 60, or in the image specifically it returns 45 for the one on the right while the one on the left is 30 even tho it should technically be 60, it always takes the max height of both labels and then uses the largest one, however this doesn't seem to work properly. @bprusinowski
here is the main logic:
export const useDualAxisTitleAdjustments = ({
leftAxisTitle,
rightAxisTitle,
containerWidth,
fontSize = TICK_FONT_SIZE,
}: useDualAxisTitleAdjustmentsProps): AxisTitleAdjustments => {
const axisTitleWidthLeft =
getTextWidth(leftAxisTitle, { fontSize }) + TICK_PADDING;
const axisTitleWidthRight =
getTextWidth(rightAxisTitle, { fontSize }) + TICK_PADDING;
const axisTitleHeightLeft = getTextHeight(leftAxisTitle, {
fontSize,
width: containerWidth / 2,
lineHeight: "15px",
});
const axisTitleHeightRight = getTextHeight(rightAxisTitle, {
fontSize,
width: containerWidth / 2,
lineHeight: "15px",
});
const maxAxisLabelHeight = Math.max(
axisTitleHeightRight,
axisTitleHeightLeft
);
const isOverlapping =
axisTitleWidthLeft + axisTitleWidthRight > containerWidth;
const overlapAmount =
(axisTitleWidthLeft + axisTitleWidthRight) / containerWidth;
const axisTitleAdjustment = maxAxisLabelHeight + TITLE_VPADDING * 2;
console.log(axisTitleHeightLeft, axisTitleHeightRight);
const topMarginAxisTitleAdjustment = 60 + axisTitleAdjustment;
return {
axisLabelHeight: maxAxisLabelHeight,
axisTitleAdjustment,
topMarginAxisTitleAdjustment,
isOverlapping,
overlapAmount,
};
};
This PR:
cc: @bprusinowski