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] Remove all overlays of a given bpmn shape element #1186

Merged
merged 13 commits into from
Mar 26, 2021
Merged

[FEAT] Remove all overlays of a given bpmn shape element #1186

merged 13 commits into from
Mar 26, 2021

Conversation

csouchet
Copy link
Member

@csouchet csouchet commented Mar 19, 2021 •
edited
Loading

Covers #1168

@csouchet csouchet added depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first BPMN diagram usability Something about the way we can interact with BPMN diagrams mvp Formerly used for bpmn-visualization MVP issues/PRs labels Mar 19, 2021
@csouchet csouchet changed the title 1168 remove all overlays of a given bpmn element [FEAT] Remove all overlays of a given bpmn element Mar 19, 2021
@github-actions
Copy link

github-actions bot commented Mar 19, 2021 •
edited
Loading

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

🤖 By surge-preview

@csouchet csouchet force-pushed the 1168-Remove_all_overlays_of_a_given_BPMN_element branch 4 times, most recently from b3c5c27 to d6e4da2 Compare March 19, 2021 17:05
@csouchet csouchet added the enhancement New feature or request label Mar 19, 2021
@csouchet csouchet force-pushed the 1168-Remove_all_overlays_of_a_given_BPMN_element branch from d6e4da2 to face75f Compare March 22, 2021 08:48
@github-actions
Copy link

github-actions bot commented Mar 22, 2021 •
edited
Loading

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

🤖 By surge-preview

@tbouffard tbouffard mentioned this pull request Mar 22, 2021
Merged
@csouchet csouchet force-pushed the 1168-Remove_all_overlays_of_a_given_BPMN_element branch from face75f to f854f0d Compare March 22, 2021 14:00
@csouchet csouchet removed the depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first label Mar 22, 2021
@csouchet csouchet force-pushed the 1168-Remove_all_overlays_of_a_given_BPMN_element branch 2 times, most recently from 9835849 to c2dd528 Compare March 24, 2021 10:31
@csouchet csouchet changed the title [FEAT] Remove all overlays of a given bpmn element [FEAT] Remove all overlays of a given bpmn shape element Mar 24, 2021
@csouchet csouchet force-pushed the 1168-Remove_all_overlays_of_a_given_BPMN_element branch 12 times, most recently from 10e699c to 1065c91 Compare March 25, 2021 16:04
@csouchet csouchet marked this pull request as ready for review March 25, 2021 16:05
@csouchet csouchet requested review from aibcmars and tbouffard and removed request for aibcmars March 25, 2021 16:05
@csouchet csouchet force-pushed the 1168-Remove_all_overlays_of_a_given_BPMN_element branch from 1065c91 to 63e1afd Compare March 26, 2021 13:53
tbouffard
tbouffard reviewed Mar 26, 2021
View reviewed changes
src/component/registry/bpmn-elements-registry.ts
*
* @param bpmnElementId The BPMN id of the element where to remove the overlays
*/
removeAllOverlays(bpmnElementId: string): void {
Copy link
Member

Choose a reason for hiding this comment

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

❓ could be just name the method removeOverlays to prepare #1178 ? to identify which overlays to remove, we will only have to pass an additional optional paramater without having to change the API or add a new method

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could rename it when we do #1178, because for now, we remove only all overlays, and I didn't think about how to implement it.

Copy link
Member

Choose a reason for hiding this comment

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

That's true, we don't know how we are going to implement the removal of specific overlays (we may not implement it soon as well).
So ok to keep this method name but in the TSDoc, can we put an @experimental annotation and state that this method is subject to name change when will implement #1178? That way we already warn users of a possible breaking change

Copy link
Member

Choose a reason for hiding this comment

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

done in ad12c6f

test/integration/helpers/html-utils.ts
expectSvgElementClassAttribute(svgGroupElement, HtmlElementLookup.computeClassValue('bpmn-service-task', additionalClasses));
expectSvgElementClassAttribute(svgGroupElement, HtmlElementLookup.computeClassValue('bpmn-service-task', options?.additionalClasses));

const overlayGroupElement = document.querySelector<SVGGElement>(`#${this.bpmnVisualization.graph.container.id} > svg > g > g:nth-child(3) > g[data-bpmn-id="${bpmnId}"]`);
Copy link
Member

Choose a reason for hiding this comment

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

ℹ️ I am working on a refactoring PR that moves the query selectors along with other selectors used in tests. Both PR are changing the code, so merge conflicts are going to happen.

@tbouffard tbouffard mentioned this pull request Mar 26, 2021
Closed
@tbouffard tbouffard mentioned this pull request Mar 26, 2021
Merged
@sonarqubecloud
Copy link

Kudos, SonarCloud 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 Mar 26, 2021
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 👍🏿

@tbouffard tbouffard merged commit ecc308b into master Mar 26, 2021
@tbouffard tbouffard deleted the 1168-Remove_all_overlays_of_a_given_BPMN_element branch March 26, 2021 17:54
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

@aibcmars aibcmars Awaiting requested review from aibcmars

Assignees
No one assigned
Labels
BPMN diagram usability Something about the way we can interact with BPMN diagrams enhancement New feature or request mvp Formerly used for bpmn-visualization MVP issues/PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@csouchet @tbouffard