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 strokestyle attribute to Box #3346

Merged
merged 9 commits into from Nov 7, 2023

Conversation

nHackel
Copy link
Contributor

@nHackel nHackel commented Nov 1, 2023

Hello! I've recently used the Box with cornerradius to create a plot and wanted to adapt the stroke style and found no option to do that.

Description

This PR adds the strokestyle keyword to a Box:
box

For the implementation I also had to adapt a check for compatible linestyles in CairoMakie, which was incompatible with the return type of convert_attribute for key"linestyle.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@jkrumbiegel
Copy link
Collaborator

jkrumbiegel commented Nov 1, 2023

Hi, thanks for the PR! I think we call that property linestyle everywhere, even if other properties have stroke as a prefix. But might as well keep in line with that linestyle convention for now

@nHackel
Copy link
Contributor Author

nHackel commented Nov 1, 2023

Ah, okay! I'm essentially building my first more involved Makie figure and haven't used many keywords so far.

In src/makielayout/types.jl I saw a few types with x- prefix but they also seem to have multiple linestyles per type. I can adapt it to linestyle.

Do you think image reference tests are necessary and if so could you point me in the right direction? I was mainly following your rounded corner PR and haven't strayed too far from files you changed there, so a bit unsure of the testing is setup

@jkrumbiegel
Copy link
Collaborator

jkrumbiegel commented Nov 2, 2023

Just renaming strokestyle to linestyle will be enough here.

Reference images are good practice, it's hard to unit-test visual attributes otherwise. You could add a test for Box here https://github.com/MakieOrg/Makie.jl/blob/master/ReferenceTests/src/tests/figures_and_makielayout.jl, just a Figure with a couple boxes, each of which exercises some of the functionality. And it's good to make the differences salient, like using thick strokes, big rounded corners, etc, so that visual changes are picked up well.

Copy link
Collaborator

@jkrumbiegel jkrumbiegel left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@jkrumbiegel jkrumbiegel merged commit 531d42d into MakieOrg:master Nov 7, 2023
14 checks passed
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.

None yet

3 participants