GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||
GeezerVison is showing github.com — domainpad: pixel spacing inside axis domains by SharadhNaidu · Pull Request #7965 · plotly/plotly.js Conversation
|
|
@emilykl apologies if i am violating any principles by directly tagging you , but could we close this issue ? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
any updates on this PR ? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry that this got dropped. What issue are you asking to close? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
oh my bad i meant the PR and the issue i am talking about is #7835 . |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
You want to close the PR? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
no,sorry for the confusion , I don't want to close it. I meant i would like the PR to be reviewed so that , once it's merged, it closes #7835. I'm also pushing a few updates today the regenerated TypeScript types that the generated types drift check is flagging, a fix so spikemode 'across' spikes stop at the padded plot area , tests for the scattergl and splom code paths , and an image mock. I'll update the description when that's in. the API questions at the bottom are still the main thing it would help if i get a input on. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Understood. I'll check with my team and follow up with you. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
thanks mate! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
updateDomain multiplied the domain by (newSpan / inputSpan). Without padding, that ratio is 1 / factor, but the multiply can change the last bit of the result, so a plot with no domainpad could move by a tiny amount. Divide by `factor` as main does, and change the divisor only when the axis has padding. Without domainpad, the result is now the same as on main.
… and types The comments and attribute descriptions now follow the Simplified Technical English rules in AGENTS.md. Lines that this PR touches now use const, let and arrow functions. There is no change in behavior. The regenerated schema.d.ts and plot-schema.json fix the generated-types-drift check, which failed because the types did not include domainpad.
Spikes with spikemode 'across' got their span from the counter axis domains, so with domainpad they crossed the reserved space, for example a subplot title. Get the span from the domain minus the padding of each counter axis. Without domainpad the span is the same as before. A free axis still extends the span to its position. The new test fails without this fix.
scattergl and splom build their regl viewports from `domain`, so they add the padding in their own code. These tests make sure that the viewport, the splom cells and the regl grid match the padded plot area.
The mock shows four subplots with different domainpad sides, so a change in the padded plot area shows in the image tests. The baseline image must come from the baselines-default-diff CI artifact.
An earlier commit removed the "with thanks to" credit by mistake. The draftlog convention includes it.
# Conflicts: # test/jasmine/tests/hover_spikeline_test.js
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hey @SharadhNaidu thanks for submitting this and sorry for the review delay.
This is generally working well on my end.
Most of the suggestions just remove superfluous comments. Otherwise, it's looking good!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Remove the inline comments flagged in review, and describe in the domainpad attribute that an overlaying axis uses the domainpad of the axis it overlays. Regenerate the plot schema and the generated types.
Give the overlaying axis in the domainpad test a conflicting domainpad, so the test covers the behavior that the attribute description now documents.
|
thanks for the review @KoolADE85 ! i have applied all the suggestions and removed the comments . for the overlaying axis one , i moved the note in to the domainpad attribute description that a axis which sets overlaying ignore its own domainpad and use the one of the axis it overlays , and regenerated the schema and types . also i gave yaxis2 it's own domainpad in the existing overlaying test so now the test is actually checking that behaviour . |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
[FEATURE]: pixel-based spacing for subplot domains (so titles can't overlap the plot above)
There was an error while loading. Please reload this page.