Add CartesianLayerPaddingProvider #941
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tyler-Lopez
force-pushed
the
lopez/layer-padding-extra-store
branch
from
bc94a52
to
666b9e3
Compare
Add CartesianLayerPaddingProvider This commit introduces CartesianLayerPaddingProvider, a means to retrieve CartesianLayerPadding with an `ExtraStore`. This commit does not substantially conflict with pre-existing functionality as `CartesianLayerPaddingProvider.fixed` is identical.
Tyler-Lopez
force-pushed
the
lopez/layer-padding-extra-store
branch
from
666b9e3
to
f6847a7
Compare
Gowsky
force-pushed
the
master
branch
3 times, most recently
from
c357b92
to
05150c4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
This commit introduces CartesianLayerPaddingProvider, a means to retrieve CartesianLayerPadding with an
ExtraStore. This commit does not substantially conflict with pre-existing functionality asCartesianLayerPaddingProvider.fixedis identical.Motivation
CartesianLayerPaddingwas recently added. This allows clients to provide padding to the start and end of a chart. However, these values are provided without anExtraStore. Therefore, I believe that the values provided will never be synchronously safe.With an
ExtraStore, we can calculate the correct spacing safely.Future Work
The discovery of the desire to change this parameter came from some advanced requirements with points.
#939
I would like to move the horizontal and vertical insets achieved through
getLargestPointto being client-provided inCartesianLayerPadding. The basic demand for that is that currently,getLargestPoint+unscalableStartPaddingcan result in an unnecessary amount of padding. Further, imagine that we only had a point at the very middle of the graph. We should, in that case, not inset the chart at all to account for the point horizontally. We should do the top if it is the maximum value in the series.So, I would also like to introduce
unscalableTopPaddingandunscalableBottomPaddingand changegetLargestPointto only impact the point spacing.Note
I could be completely incorrect in the basic assertion here that this is necessary to correctly have synchronous padding: for example, to avoid a bad state if the padding, which is dependent on the line thickness, changes before the transaction finishes. Please correct me if I am wrong.