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.

Sign up for GitHub

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

Jump to bottom

feat(style): add support for fill color gradient in the Update Style API #2760

Merged
merged 20 commits into from
Jul 12, 2023
Merged

feat(style): add support for fill color gradient in the Update Style API #2760

merged 20 commits into from
Jul 12, 2023

Conversation

csouchet
Copy link
Member

@csouchet csouchet commented Jul 10, 2023 •
edited by tbouffard
Loading

This pull request introduces the ability to use fill color gradients in the Update Style API, improving customization for shapes.

Major changes include:

  • Introduction of new types, FillColorGradient and Direction, to handle gradient fill colors.
    The FillColorGradient type defines properties for the startColor, endColor and direction.
    The Direction' type provides options such as left-to-rightandbottom-to-top` for specifying the gradient direction.
  • Updated the existing color' property of the Fill' type, to accept either a solid color string, or a new `FillColorGradient' type.
  • Changed the functionality for resetting the fill color property. Currently, resetting the fill color removes the gradient.
  • Added visual and integration tests.
  • Updated the TS documentation to clearly explain the new gradient type.
  • Modified the "elements identification" page to display the CallActivity with the fill gradient color.

image

Closes #2559

@csouchet csouchet added enhancement New feature or request BPMN diagram styling Change the standard rendering: stroke color, thickness labels Jul 10, 2023
@csouchet csouchet force-pushed the 2559-Add_support_for_fill_color_gradient_in_the_Update_Style_API branch from afcd449 to b1c8756 Compare July 10, 2023 15:47
@csouchet csouchet force-pushed the 2559-Add_support_for_fill_color_gradient_in_the_Update_Style_API branch from 7b1b14c to 91b90b3 Compare July 11, 2023 13:53
@csouchet csouchet marked this pull request as ready for review July 12, 2023 08:10
@csouchet csouchet requested a review from tbouffard July 12, 2023 08:10
@github-actions
Copy link

github-actions bot commented Jul 12, 2023 •
edited
Loading

🎊 PR Preview 542e307 has been successfully built and deployed to https://process-analytics-bpmn-visualization-js-demo_preview-pr-2760.surge.sh

🕐 Build time: 0.017s

🤖 By surge-preview

tbouffard
tbouffard reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

LGTM, I only have minor suggestions for changes

✔️ implementation and tests cover all use cases
✔️ elements-identification.html OK (tested with the surge preview)

src/component/registry/types.ts
* {@link https://jgraph.github.io/mxgraph/docs/js-api/files/util/mxConstants-js.html#mxConstants.STYLE_GRADIENTCOLOR}
*
* **Notes**:
* Using the same color for the start color and end color will have the same effect as setting only the fill color with an HTML color name, HEX code or special keyword.
Copy link
Member

Choose a reason for hiding this comment

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

praise: 👍🏿

src/component/mxgraph/style/utils.ts Show resolved Hide resolved
src/component/mxgraph/style/utils.ts Outdated Show resolved Hide resolved
src/component/mxgraph/style/utils.ts Outdated Show resolved Hide resolved
test/integration/matchers/toBeShape/index.ts
Comment on lines +85 to +88
expectedModel.isSwimLaneLabelHorizontal && (style.horizontal = Number(expectedModel.isSwimLaneLabelHorizontal));

// ignore marker order, which is only relevant when rendering the shape (it has its own order algorithm)
'markers' in expectedModel && (style.markers = expectedModel.markers.sort());
style.markers = expectedModel.markers?.sort();
Copy link
Member

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.

Copy link
Member Author

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 the markers property set to undefined if we either don't set it or explicitly set it to undefined. Since it is the same returned object, this way of expressing it is easier to read 🙂

test/integration/helpers/model-expect.ts
Comment on lines +169 to +172
export interface ExpectedFill {
color?: string;
opacity?: Opacity;
}
Copy link
Member

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.

Copy link
Member Author

@csouchet csouchet Jul 12, 2023 •
edited
Loading

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 and mxgraph, I needed to change it.

test/integration/mxGraph.model.style.api.test.ts Outdated Show resolved Hide resolved
test/integration/mxGraph.model.style.api.test.ts Outdated Show resolved Hide resolved
test/shared/visu/bpmn-page-utils.ts Outdated Show resolved Hide resolved
test/shared/visu/bpmn-page-utils.ts Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jul 12, 2023 •
edited
Loading

♻️ PR Preview 542e307 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

tbouffard
tbouffard approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks

@csouchet csouchet merged commit 7ecdd37 into master Jul 12, 2023
@csouchet csouchet deleted the 2559-Add_support_for_fill_color_gradient_in_the_Update_Style_API branch July 12, 2023 15:48
@tbouffard tbouffard mentioned this pull request Jul 19, 2023
Open
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbouffard tbouffard tbouffard approved these changes

Assignees
No one assigned
Labels
BPMN diagram styling Change the standard rendering: stroke color, thickness enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEAT] Add support for fill color gradient in the Update Style API
2 participants
@csouchet @tbouffard