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

Fixing Popover and XAxis ticks Tooltip bugs for Vertical bar chart and Line Chart #32760

Open
wants to merge 10 commits into
base: usr/atisjai/chartsV9
Choose a base branch
from

Conversation

srmukher
Copy link
Contributor

Following bugs were fixed:

  1. VC Axis tooltip - tooltip is overlapping with other labels
  2. VBC date axis - y values not showing on callout
  3. VBC only bars - x values and shape not showing on callout

@srmukher srmukher requested a review from a team as a code owner September 10, 2024 10:00
marginTop: '13px',
color: tokens.colorNeutralForeground2,
borderLeft: 'var(--lineColor)',
Copy link
Contributor

Choose a reason for hiding this comment

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

what value does lineColor represent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the value is provided in the popover class as 4px solid ${color},

@srmukher srmukher changed the title Fixing Popover bugs for Vertical bar chart Fixing Popover and XAxis ticks Tooltip bugs for Vertical bar chart and Line Chart Sep 12, 2024
xAxis: xAxisElement,
};
xAxisElement && tooltipOfXAxislabels(tooltipProps);
setXAxisElement(xAxisElement.node());
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of setting xAxisElement can you set the tooltipprops here directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tooltip props now has only these 2 elements out of which tooltipRef is not a state variable, thus need not be set. const tooltipProps = {
xAxis: xAxis,
div: tooltipRef.current,
};

@@ -1,58 +0,0 @@
import { makeStyles, mergeClasses, shorthands } from '@griffel/react';
Copy link
Contributor

Choose a reason for hiding this comment

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

dont remove this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but no styles are used from here. It will be empty then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added as discussed

@@ -816,27 +827,10 @@ export const LineChart: React.FunctionComponent<ILineChartProps> = React.forward
</g>,
);
}
const classes = useLineChartStyles_unstable(props);
// Removing un wanted tooltip div from DOM, when prop not provided.
if (!props.showXAxisLablesTooltip) {
Copy link
Contributor

Choose a reason for hiding this comment

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

where is the code eq to this logic when showXAxisLabelsTooltip is false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do not need that as we are setting the props only when showXAxisLablesTooltip is true. Earlier it was removing the div from dom for which this block was kept.

verticalbarchartClassNames.opacityChangeOnHover,
baseStyles.opacityChangeOnHover /*props.styles?.opacityChangeOnHover*/,
),
opacityChangeOnHover: mergeClasses(verticalbarchartClassNames.opacityChangeOnHover),
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was giving build error as opacityChangeOnHover is not present in baseStyles

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants