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

Path pattern fills #7280

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

Path pattern fills #7280

wants to merge 9 commits into from

Conversation

alexcjohnson
Copy link
Collaborator

Arbitrary fill patterns! For example geologists have all sorts of standardized patterns they know and love, and this lets you create any pattern you want. It still has the constraint of two colors (foreground and background) and a square unit cell, but now any svg path string is accepted to define the foreground color area.

I chose to do this by adding a new attribute in the pattern container, path. We could instead put the path into the shape attribute - in fact we only accept one (shape takes precedence) and for drawing I combine them into one anyway. And this is what we do for line.dash for example, there's an enumerated set but you also have the option of providing an arbitrary string in the same place. On the other hand layout.shapes has a path attribute for type='path' shapes and other attributes for other shapes. Also line.dash is somewhat awkward to describe (a string attribute that nevertheless lists values like an enum?) and shape='<path string>' is a bit funny to read. I don't feel strongly about this though - if others would prefer to add this into the shape attribute that would also be fine.

@alexcjohnson alexcjohnson added the feature something new label Nov 22, 2024
"bgcolor": "black"
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to keep pattern_bars.json as it was before.
Please duplicate the mock and name the new mock zz-pattern_bars-path.json.

This is wonderful feature.
Thanks @alexcjohnson for the PR. 🏆

We may introduce new features like this one in v3.1.0 not v3.0.0; even though one may argue that it's a low risk feature. BTW it requires extra testing on the python side, etc.
cc: @gvwilson

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK - personally I think the mocks are getting a little excessive, and we'd be better off testing as many things as we can in one mock, which is why I just added this to one of the existing mocks. But I've made the change.

(
(nestedSchema.coerceNumber && valIn !== +valOut) ||
(!isArrayOrTypedArray(valIn) && valIn !== valOut) ||
(String(valIn) !== String(valOut))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we're comparing the original data array to something we coerced off a deep copy, arrays that make it here will never be ===

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants