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

fix: DH-18279: fix one click titles #2381

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jnumainville
Copy link
Contributor

@jnumainville jnumainville commented Mar 5, 2025

Example from DH-18279

import deephaven.plot.express as dx
from deephaven.plot.figure import Figure
from deephaven.plot.selectable_dataset import one_click

stocks = dx.data.stocks()

one_click = Figure().plot_xy(series_name="Stock Prices", t=one_click(stocks, ["Sym"]), x="Timestamp", y="Price").show()

Title now updates as the input filter is updated

@jnumainville jnumainville requested review from a team, Copilot and bmingles and removed request for a team March 5, 2025 19:28

Choose a reason for hiding this comment

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

PR Overview

A fix to ensure one click titles update correctly by adjusting title padding and triggering layout update events properly.

  • Updated the setTitle method to recalculate margins based on title content.
  • Added a test to verify that the title update event is fired exactly once.

Reviewed Changes

File Description
packages/chart/src/FigureChartModel.test.ts Added test to ensure title update events are triggered correctly.
packages/chart/src/FigureChartModel.ts Modified setTitle to adjust margins and trigger layout update events.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

packages/chart/src/FigureChartModel.ts:629

  • The title update event appears to be fired twice in the setTitle method, which may lead to unintended side effects and fails the test expectation. Consider removing the duplicate call so that the event is fired only once.
this.fireLayoutUpdated({ title: this.layout.title });
Copy link

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.80%. Comparing base (d44370b) to head (764c89f).

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2381    +/-   ##
========================================
  Coverage   46.79%   46.80%            
========================================
  Files         710      710            
  Lines       39234    39235     +1     
  Branches     9976     9791   -185     
========================================
+ Hits        18361    18365     +4     
- Misses      20819    20859    +40     
+ Partials       54       11    -43     
Flag Coverage Δ
unit 46.80% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant