Skip to content

feat(react-charting): Support for Funnel Chart #34688

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

srmukher
Copy link
Contributor

@srmukher srmukher requested a review from a team as a code owner June 20, 2025 16:03
Copy link

github-actions bot commented Jun 20, 2025

📊 Bundle size report

✅ No changes found

@@ -0,0 +1,7 @@
{

Choose a reason for hiding this comment

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

🕵🏾‍♀️ visual changes to review in the Visual Change Report

vr-tests-react-components/Drawer 1 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests-react-components/Drawer.overlay drawer full - High Contrast.chromium.png 6688 Changed
vr-tests-react-components/Positioning 2 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests-react-components/Positioning.Positioning end.chromium.png 712 Changed
vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png 725 Changed
vr-tests-react-components/TagPicker 2 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests-react-components/TagPicker.disabled - High Contrast.chromium.png 1321 Changed
vr-tests-react-components/TagPicker.disabled - Dark Mode.disabled input hover.chromium.png 659 Changed
vr-tests/react-charting-AreaChart 1 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests/react-charting-AreaChart.Custom Accessibility.default.chromium.png 11 Changed
vr-tests/react-charting-GaugeChart 1 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests/react-charting-GaugeChart.Basic.default.chromium.png 2 Changed
vr-tests/react-charting-LineChart 4 screenshots
Image Name Diff(in Pixels) Image Type
vr-tests/react-charting-LineChart.Events.default.chromium.png 1 Changed
vr-tests/react-charting-LineChart.Multiple.default.chromium.png 192 Changed
vr-tests/react-charting-LineChart.Multiple - RTL.default.chromium.png 200 Changed
vr-tests/react-charting-LineChart.Multiple - Dark Mode.default.chromium.png 181 Changed

There were 2 duplicate changes discarded. Check the build logs for more information.

@AtishayMsft
Copy link
Contributor

what is the size increase for DeclarativeChart with this addition.

orientation: 'horizontal',
};

function renderSegmentText({
Copy link
Contributor

Choose a reason for hiding this comment

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

The segment text color has to ensure color contrast. See

foregroundColor = this._getInvertedTextColor(foregroundColor);

culture,
onMouseOver: event => _handleStackedHover(cur.stage as string, v, event),
onMouseMove: event => _handleStackedHover(cur.stage as string, v, event),
onMouseOut: () => _handleMouseOut(),
Copy link
Contributor

Choose a reason for hiding this comment

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

put all these props in an object called eventHandlerProps and reuse everywhere

alignmentBaseline="middle"
onMouseOver={onMouseOver}
onMouseMove={onMouseMove}
onMouseOut={onMouseOut}
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

if (orientation === 'horizontal') {
for (let i = 0; i < stages.length; i++) {
const cur = stages[i];
for (let k = 0; k < (cur.subValues ?? []).length; k++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for (let k = 0; k < (cur.subValues ?? []).length; k++) {

can the code inside double for loop be converted into 1 function

@AtishayMsft
Copy link
Contributor

AtishayMsft commented Jun 21, 2025

1 issue. The text in the bottom segment is not in the center vertically.
Same issue for vertical case also

image

@AtishayMsft
Copy link
Contributor

Another issue: Multi legend selection is not working

@AtishayMsft
Copy link
Contributor

Highlighting on chart on stacked case is making all segments greyed out.

image

@AtishayMsft
Copy link
Contributor

Could you add all these cases in contrib repo

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.

3 participants