standalone Plot.scale respects the transform and percent options - #2458
Open
eastagiletracker wants to merge 1 commit into
Open
standalone Plot.scale respects the transform and percent options#2458eastagiletracker wants to merge 1 commit into
eastagiletracker wants to merge 1 commit into
Conversation
This file contains hidden or 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
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.
This PR proposes making the standalone
Plot.scalerespect the transform and percent scale options, so that it materializes the same options as plot.scale (fixes #2325). We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/456. You can sign in with your GitHub ID to claim ownership of the project.The defect
createScalespopulates the generic scale options — percent and transform — onto the scale descriptor aftercreateScalereturns, which is whyplot(…).scale(name)exposes them. The standalonePlot.scalegoes straight fromnormalizeScaletoexposeScaleand never runs that step, so both options are silently dropped from the returned scale. At356f579onmain:This is not only cosmetic on the returned object. The scales documentation says a standalone scale object can be passed to
Plot.plotas the corresponding scale options, andapplyScaleTransforminsrc/plot.jsreads transform and percent back out of those options — so a plot that reuses a standalone scale renders untransformed data, while the identical inline options transform it. That asymmetry is what #2325 reports.The change
The percent and transform handling moves out of
createScalesinto a smallapplyScaleOptionshelper that bothcreateScalesandscalecall, so the two paths materialize scale options identically instead of one of them re-implementing the other.Plot.plotbehavior is unchanged: the helper keeps the existing ordering (percent is applied before the label is inferred, so the “(%)” label suffix still works) and the existinginvalid scale transform; not a functionerror, which the standalone path now raises as well.Verification
test/scales/scales-test.jsgains four tests: the exposed transform, the exposed percent, the not-a-function transform error, and an end-to-end check that a plot handed a standalone scale renders byte-identical output to the same plot handed the equivalent inline options. All four fail onmainand pass here.ESLint and Prettier are clean on both trees, and the 28 existing snapshots carrying a “(%)” axis label are unchanged, which is what covers the reordered label inference. One note on merge order: #2427 edits the same line of
scale, so whichever of the two lands second needs a one-line rebase.How this was managed
We ran this work on a board imported from this repository's own issues and pull requests (1875 stories). The story covering this fix is Plot.scale(…) ignores the transform option, on the board at https://eastagiletracker.com/projects/456.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com