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 relative gradient option to the baseline series #1730

Open
wants to merge 3 commits into
base: v5-candidate
Choose a base branch
from

Conversation

illetid
Copy link
Contributor

@illetid illetid commented Nov 18, 2024

Type of PR: enhancement

PR checklist:

Overview of change:
Added a new option to the baseline series where we calculate gradient from the visible points range and not from the top and bottom of the chart.
Example with two series with the same settings. Top and bottom colors are different without option enabled
without option:
image

with the option enabled:
image

Copy link

github-actions bot commented Nov 18, 2024

size-limit report 📦

Path Size
ESM 42.42 KB (+0.16% 🔺)
ESM createChart 36.27 KB (0%)
ESM createChartEx 35 KB (-0.17% 🔽)
ESM Standalone 43.96 KB (+0.35% 🔺)
Standalone 43.88 KB (+0.3% 🔺)
Plugin: Text Watermark 1.86 KB (0%)
Plugin: Image Watermark 1.68 KB (-0.18% 🔽)
Plugin: Series Markers 3.89 KB (-0.13% 🔽)
Series: LineSeries 2.54 KB (-0.04% 🔽)
Series: BaselineSeries 3.17 KB (+3.87% 🔺)
Series: AreaSeries 3.1 KB (+3.87% 🔺)
Series: BarSeries 2.15 KB (+0.28% 🔺)
Series: CandlestickSeries 2.43 KB (-0.25% 🔽)
Series: HistogramSeries 2.2 KB (+0.18% 🔺)

@illetid illetid marked this pull request as ready for review November 18, 2024 10:51
@illetid illetid self-assigned this Nov 18, 2024
Copy link
Contributor

@SlicedSilver SlicedSilver left a comment

Choose a reason for hiding this comment

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

I think we should add this option to the area series as well.

Additionally, could you check the behaviour when you have more than one series on the same price scale?
because there would be two ways to implement this feature:

  • Consider the min and max visible values of items (like this PR)
  • Use the min and max values from the scale margins

For a single series (and the price scale not adjusted) then the visible range and the scale margins range would essentially be the same and unchanging (and quicker to calculate).

However for multiple series, this wouldn't be the case. So please try these different setups and comment on how they look.

.size-limit.js Outdated
@@ -79,7 +79,7 @@ export default [
path: 'dist/lightweight-charts.production.mjs',
import: '{ BaselineSeries }',
ignore: ['fancy-canvas'],
limit: '3.2 KB',
limit: '3.3 KB',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maybe make the limit a bit larger.
The CI will add a comment to each PR with the size changes therefore we don't need this limit here to be so close to the actual amount but can rather be something like 4 KB

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 the same option for the Area series. Screenshots from the PR's description represent two series on the same price scale.
I think the approach of calculating gradients based on the visible range is a better solution, because the scale margins work only for the one series and if applied margins in case if there are two or more series gradient will be shifted at the value of margin and still be inconsistent between series. with min/max values we don't care about the settings or other series on the chart, we calculate based only on the current series visible data. In case if someone has performance concerns this behavior can be disabled.
I made it false by default, WDYT, maybe it should be enabled by default?

Copy link
Contributor

@SlicedSilver SlicedSilver left a comment

Choose a reason for hiding this comment

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

I have a concern that there are graphics tests (which haven't been changed) are failing for the area series. Could you please investigate why the area series is rendering differently in these cases. The opacity seems to be different.

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