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

add pane index to price scale api #1821

Merged
merged 9 commits into from
Feb 26, 2025
Merged

add pane index to price scale api #1821

merged 9 commits into from
Feb 26, 2025

Conversation

illetid
Copy link
Contributor

@illetid illetid commented Feb 19, 2025

Type of PR:

PR checklist:

Overview of change:
Added pane index as parameter to get price scale api. And fixed issue with getting correct price scale when series in not on the 1st pane

@illetid illetid marked this pull request as draft February 20, 2025 10:00
Copy link

github-actions bot commented Feb 24, 2025

size-limit report 📦

Path Size
ESM 44.92 KB (+0.15% 🔺)
ESM createChart 36.9 KB (+0.14% 🔺)
ESM createChartEx 35.6 KB (-0.04% 🔽)
ESM createYieldCurveChart 37.12 KB (+0.16% 🔺)
ESM createOptionsChart 35.84 KB (+0.14% 🔺)
Standalone-ESM 46.39 KB (+0.05% 🔺)
Standalone 46.31 KB (+0.08% 🔺)
Plugin: Text Watermark 1.87 KB (+0.21% 🔺)
Plugin: Image Watermark 1.69 KB (+0.18% 🔺)
Plugin: Series Markers 3.99 KB (+0.4% 🔺)
Series: LineSeries 2.54 KB (-0.08% 🔽)
Series: BaselineSeries 3.19 KB (-0.1% 🔽)
Series: AreaSeries 3.12 KB (-0.1% 🔽)
Series: BarSeries 2.14 KB (-0.05% 🔽)
Series: CandlestickSeries 2.43 KB (-0.05% 🔽)
Series: HistogramSeries 2.19 KB (+0.27% 🔺)
Plugin: UpDownMarkersPrimitive 2.37 KB (-0.5% 🔽)

@illetid illetid marked this pull request as ready for review February 25, 2025 16:19
@illetid illetid requested a review from SlicedSilver February 25, 2025 16:19
@SlicedSilver
Copy link
Contributor

Potentially minor.

The series API has the priceScale() method for getting a price scale API instance for the price scale currently used by the series. However if the series is moved to a different pane (or the panes are swapped) then the instance of the API isn't actually for that specific series anymore.

// areaSeries is on Pane 0
const areaPriceScale = areaSeries.priceScale();
chart.swapPanes(0,1);
// areaSeries is now on Pane 1
areaPriceScale.applyOptions({
	textColor: 'yellow'
}); // ← This applies to Pane 0, and thus not the price scale currently used by the area series.

Not saying that this behaviour is wrong but rather that we should at least mention it in the JSDoc comment so it is clearer. It currently says:

/**
 * Returns interface of the price scale the series is currently attached
 *
 * @returns IPriceScaleApi object to control the price scale
 */
priceScale(): IPriceScaleApi;

Perhaps we should mention (warn) that the price scale api returned is for the current price scale used by the series (id and pane index) and if the series is moved or starts to use a different id then this interface want to changed to the new price scale.

@illetid
Copy link
Contributor Author

illetid commented Feb 26, 2025

@SlicedSilver updated the jsdoc, could you take a look pls

@illetid illetid requested a review from SlicedSilver February 26, 2025 13:45
@illetid illetid merged commit a0d8774 into master Feb 26, 2025
27 of 33 checks passed
@illetid illetid deleted the fix/pricescale-multipane branch February 26, 2025 14:01
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.

[v5] changing priceScale for one pane impact all panes
2 participants