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

Changed rendering of baseline and area series when using scaleMargins #1010

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

Conversation

markeasting
Copy link

Type of PR:
Enhancement

PR checklist:

Overview of change:
This change will make the rendering of color stops in the baseline and area series gradients more consistent when using scaleMargins.

The gradient will now always reach the full color whether it is scaled to the top or bottom of the view (as I'd expect from setting its color options).

Is there anything you'd like reviewers to focus on?
The gradient is only changed when using a custom price scale, since it seems that these cannot be moved manually. Is this correct? Otherwise, the gradient will go into negative values, giving unwanted results.

Furthermore, I'm using the code below to check whether or not we are on a custom price scale:

const isCustomScale = priceScale.id() !== 'right' && priceScale.id() !== 'left';

Not sure if this is the best method, open for suggestions. Maybe use the DefaultPriceScaleId enum?

Copy link
Contributor

@timocov timocov left a comment

Choose a reason for hiding this comment

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

See comments. Also, could you add tests for this feature please? Let me know if you need any help or additional information.

@@ -32,3 +32,6 @@ debug.html

# graphics tests out data
/tests/e2e/graphics/.gendata/

# MacOS
.DS_Store
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't mind let's revert this. This folder should in your global gitignore since it has no in common with the project itself.

Comment on lines +48 to +49
bottom = height * (1 - priceScaleProps.scaleMargins.bottom);
top = height * priceScaleProps.scaleMargins.top;
Copy link
Contributor

Choose a reason for hiding this comment

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

well, I don't think that it should be like that. Instead, we need to get min/max coordinates for a visible range of this series. If we'll do this we won't need to check whether it is a "custom" scale or not because it will work automatically for any series. See my comment here #1005 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

we have this._items where we can get all coordinates for each item, but please make sure that you use this._itemsVisibleRange to iterate over that array properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

also, you could take a look at _convertToCoordinates method, which is used to update coordinates, probably we could change override it here and calculate the data only once and save it somewhere.

@SlicedSilver SlicedSilver added this to the 4.1 milestone Feb 9, 2023
@SlicedSilver
Copy link
Contributor

Updates:

  • This change of behavior should be an option.
  • Need to perform an investigation on the desired behavior when margins are negative.

@SlicedSilver SlicedSilver modified the milestones: 4.1, 5.0 Sep 22, 2023
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.

Baseline series should use min/max price range in visible range to calculate gradient stops
3 participants