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 support for xref/yref paper for shapes with layer "between" #6989

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

Conversation

my-tien
Copy link
Contributor

@my-tien my-tien commented Apr 30, 2024

Previously, a shape with layer "between" and xref/yref "paper" fell back to layer = "below".

To support paper refs, I split up the cartesianLayer into a cartesianLayerBelow (where plot background and gridlines are drawn) and cartesianLayerAbove (where the rest is drawn). Between the two is a new layerBetween.

Disclaimer I am required to add that…

the software is provided "as is", without warranty of any kind, express or implied, including but not limited to the warranties of merchantability, fitness for a particular purpose and noninfringement. in no event shall the authors or copyright holders be liable for any claim, damages or other liability, whether in an action of contract, tort or otherwise, arising from, out of or in connection with the software or the use or other dealings in the software.

src/plot_api/plot_api.js Outdated Show resolved Hide resolved
@archmoj archmoj added feature something new status: has TODOs community community contribution labels May 1, 2024
@my-tien
Copy link
Contributor Author

my-tien commented May 6, 2024

@archmoj I finally fixed all failing tests except for baselines. Could you have a look again? :)

@my-tien
Copy link
Contributor Author

my-tien commented May 10, 2024

Again, remaining failing tests are unrelated baseline images (I think).

@my-tien
Copy link
Contributor Author

my-tien commented May 21, 2024

@archmoj Please let me know if there is anything else I should look into before the PR can be approved.

@archmoj
Copy link
Contributor

archmoj commented Jun 10, 2024

Please fetch upstream/master and merge it here.
Then please investigate the baseline changes in the failing tests.
Thank you!

@my-tien
Copy link
Contributor Author

my-tien commented Jun 19, 2024

It looks like the differing baseline tests just have a few pixel changes. I had a closer look at bar_multiline_labels.png because the image diff there seemed to contain more changes. I have attached a diff of the SVG between master and this branch and at least to me it looks exactly as intended:
All the sections with the background grid and zeroline are moved out of the previous "subplot xnyn" block into "subplot xnyn-below" and after all the "-below" layers comes the new "layer-between"
diff_bar_multiline_labels.patch

@archmoj
Copy link
Contributor

archmoj commented Jun 25, 2024

Please resolve the conflicts in this PR.
Thank you!

Conflicts:
    src/plot_api/subroutines.js
    src/plots/cartesian/index.js
    test/jasmine/tests/splom_test.js
@my-tien
Copy link
Contributor Author

my-tien commented Jun 27, 2024

Please resolve the conflicts in this PR. Thank you!

Conflicts have been resolved, now we are back to the same failing tests again.

@gvwilson gvwilson added cs customer success P2 needed for current cycle and removed status: has TODOs labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution cs customer success feature something new P2 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants