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.

Sign up for GitHub

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

style: Improved dashboard embed style & Overlapping Axis Title for Single Line Charts #1745

Merged
merged 5 commits into from
Sep 16, 2024
Merged

style: Improved dashboard embed style & Overlapping Axis Title for Single Line Charts #1745

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
994570c
style: Improved dashboard embed style
noahonyejese Sep 13, 2024
bee36bd
fix: Improved styling using mui utlities
noahonyejese Sep 13, 2024
4fc2eff
fix: Changed showVisualizeLink conditions
noahonyejese Sep 13, 2024
9d77c4a
fix: Text Wrapping Fix for single line Charts
noahonyejese Sep 16, 2024
facb3bc
fix: Added Padding to the Legend
noahonyejese Sep 16, 2024
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
8 changes: 7 additions & 1 deletion app/charts/shared/axis-height-linear.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ export const AxisHeightLinear = () => {
textColor: labelColor,
});

const textIsOverlapping = axisTitleWidth > state.bounds.chartWidth;
const overlappingAmount = Math.ceil(axisTitleWidth / state.bounds.chartWidth);

return (
<>
{state.chartType === "comboLineSingle" ? (
Expand All @@ -61,7 +64,10 @@ export const AxisHeightLinear = () => {
{state.yAxisLabel}
</text>
) : (
<foreignObject width={axisTitleWidth} height={axisLabelFontSize * 2}>
<foreignObject
width={textIsOverlapping ? state.bounds.chartWidth : axisTitleWidth}
height={axisLabelFontSize * overlappingAmount * 2}
>
Copy link
Collaborator

@bprusinowski bprusinowski Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@noahonyejese thanks for improving this 💯

I think there's still a need to increase the chart height / margin between the axis titles and the graphical elements – now it's already better, as it wraps, but we can end up with a situation like this:

Screenshot 2024-09-16 at 08 58 56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah looks like adding a bit of padding would be ideal and not just the raw value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@noahonyejese I am not sure if this fixed the problem, I think you need to also update chart bounds to take into account the extended chart height, like you did for combo charts?

Screenshot 2024-09-22 at 13 04 37

<OpenMetadataPanelWrapper component={state.yMeasure}>
<span style={{ fontSize: axisLabelFontSize }}>
{state.yAxisLabel}
Expand Down
Loading