Skip to content

Add basis lines and process signals #81

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

Merged
merged 1 commit into from
Aug 4, 2025

Conversation

ClaudiaGivan
Copy link
Collaborator

No description provided.

@ClaudiaGivan ClaudiaGivan requested review from butsyk and Copilot August 4, 2025 07:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the charting system by adding configurable basis lines (control limits) and process signal visualization capabilities to control charts and moving range charts.

  • Refactors renderer constructors to remove avgMovingRangeFunc dependency and implement dynamic limit configuration
  • Adds support for multiple overlapping data points with enhanced tooltips and visual indicators
  • Implements process signal highlighting and limit visibility controls for better chart analysis

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
src/graphs/scatterplot/ScatterplotRenderer.js Enhanced click handling for overlapping tickets and made dash pattern configurable for horizontal lines
src/graphs/pbc/PBCRenderer.js Simplified constructor by removing avgMovingRangeFunc parameter from child renderer initialization
src/graphs/moving-range/MovingRangeRenderer.js Added configurable limit system, enhanced tooltips for overlapping data points, and improved visual indicators
src/graphs/control-chart/ControlRenderer.js Implemented comprehensive limit management, process signal visualization, and enhanced tooltip system for overlapping tickets
Comments suppressed due to low confidence (1)

src/graphs/moving-range/MovingRangeRenderer.js:32

  • The property name 'URL' is confusing in this context as it appears to represent a limit value, not a web address. Consider renaming to something more descriptive like 'upperLimit' or 'url' (lowercase) if it's an acronym.
    this.topLimit = limitData?.URL;

@ClaudiaGivan ClaudiaGivan force-pushed the basis-lines-process-signals branch from b1a7154 to f8eb62c Compare August 4, 2025 07:34
@ClaudiaGivan ClaudiaGivan merged commit 251ff52 into main Aug 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants