Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(style): add support for fill color gradient in the Update Style API #2760
feat(style): add support for fill color gradient in the Update Style API #2760
Changes from all commits
7e7495f
af27ed9
cb585b1
bbc9b1e
3220ce7
e9c6f29
b1c8756
8ede51b
11da090
69dd2f2
e618e24
7c09a06
5ee19f4
f6a00cd
51c5f51
91b90b3
1f77e36
dd09212
a5b61e0
542e307
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: 👍🏿
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: needed as in the Fill type, the color can be an object and here, the checks expect it to be a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look my first implementation for the integration tests, I used the same implementation for the update style API.
But, the calculation of what we really test was hidden from the reading of the tests.
bbc9b1e
As the
direction
is not the same value between the API andmxgraph
, I needed to change it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: this seems to be a refactoring that is not related to the new feature.
If so, please revert and eventually create a dedicated PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a rest of bbc9b1e.
The old code wasn't clear for me for the real test.
I can move in another PR, but I was lazy 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I don't understand the need for this refactoring and it changes the resulting style object.
Previously the style object didn't contain the properties, not they are set to undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The object returned by
buildExpectedShapeCellStyle
will have themarkers
property set toundefined
if we either don't set it or explicitly set it toundefined
. Since it is the same returned object, this way of expressing it is easier to read 🙂