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 gradient support #71

Merged
merged 5 commits into from
Oct 13, 2021
Merged

Add gradient support #71

merged 5 commits into from
Oct 13, 2021

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Oct 7, 2021

The model now supports having SVG gradients applied to individual chunks. A new gradients property has been added to the JSON model that contains a map of gradient IDs to definitions. A gradient is applied to a chunk by setting the optional chunk property gradient to a gradient ID.

Both linear and radial gradients are supported, along with most SVG attributes. Any attribute with a limited set of options is validated to ensure it is set correctly, but the attributes that take lengths or colors simply ensure the value is a string (validating them property would be challenging). Validation has been added to throw an error if any unsupported attributes are present.

The "gradient" demo has been updated to use this new gradient support. Now that example sets the gradient on each chunk rather than applying an overlay and masking it to match the model's shape. This greatly simplifies the example.

Also the example-specific model files have been moved into the example directory. A new model was needed for the updated gradient example, and this seemed like a better place for it to go, so that the top-level of the repository doesn't get too cluttered.

@Gudahtt Gudahtt requested a review from a team as a code owner October 7, 2021 13:37
@Gudahtt Gudahtt force-pushed the add-gradient-support branch 4 times, most recently from 4d3262e to f4e970b Compare October 7, 2021 20:46
util.js Outdated Show resolved Hide resolved
rekmarks
rekmarks previously approved these changes Oct 11, 2021
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

[redacted]

rekmarks
rekmarks previously approved these changes Oct 11, 2021
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

I'm not gonna pretend to understand how any of this works, but the code organization looks good, and the gradient example looks just the same as before.

LGTM!

@Gudahtt
Copy link
Member Author

Gudahtt commented Oct 12, 2021

This has been rebased to resolve conflicts.

I'm not gonna pretend to understand how any of this works

I'd be happy to walk through any of this, if it would be helpful! I think it could be explained easily.

rekmarks
rekmarks previously approved these changes Oct 12, 2021
util.js Show resolved Hide resolved
util.js Outdated Show resolved Hide resolved
util.js Show resolved Hide resolved
Gudahtt and others added 4 commits October 12, 2021 17:38
The model now supports having SVG gradients applied to individual
chunks. A new `gradients` property has been added to the JSON model
that contains a map of gradient IDs to definitions. A gradient is
applied to a chunk by setting the optional chunk property `gradient` to
a gradient ID.

Both linear and radial gradients are supported, along with most SVG
attributes. Any attribute with a limited set of options is validated to
ensure it is set correctly, but the attributes that take lengths or
colors simply ensure the value is a string (validating them property
would be challenging). Validation has been added to throw an error if
any unsupported attributes are present.

The "gradient" demo has been updated to use this new gradient support.
Now that example sets the gradient on each chunk rather than applying
an overlay and masking it to match the model's shape. This greatly
simplifies the example.

Also the example-specific model files have been moved into the
`example` directory. A new model was needed for the updated gradient
example, and this seemed like a better place for it to go, so that the
top-level of the repository doesn't get too cluttered.
Color and gradient are mutually exclusive options, so they should not
both be set. An error is now thrown if both are set, so that there is
no ambiguoity about how the polygon will be colored.
This attribute was supported by both types of gradient, but it was
missing from the list of common attributes.
@Gudahtt
Copy link
Member Author

Gudahtt commented Oct 12, 2021

Updated! I added JSDoc comments, and I added validation to ensure color and gradient couldn't both be specified on a chunk. Also I found a mistake in the gradient definition (gradientTransform was missing from the list).

JSDoc comments have been added for the two functions that have been
modified in this PR.
@Gudahtt Gudahtt force-pushed the add-gradient-support branch from 21f2416 to 0e3fb23 Compare October 12, 2021 22:06
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt Gudahtt merged commit 0ca4a5a into main Oct 13, 2021
@Gudahtt Gudahtt deleted the add-gradient-support branch October 13, 2021 12:18
@Gudahtt Gudahtt mentioned this pull request Oct 22, 2021
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.

2 participants