Skip to content

[FEAT] Ensure overlays are passed as a list to the js function #57

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
Merged
csouchet merged 3 commits into from
Oct 15, 2021
Merged

[FEAT] Ensure overlays are passed as a list to the js function #57

csouchet merged 3 commits into from
Oct 15, 2021

Conversation

@alachambre
Copy link
Contributor

@alachambre alachambre commented Oct 11, 2021

The javascript in inst/htmlwidgets/bpmnVisualization.js is expecting a list of overlays (l.29)
-> If the user passes a single overlay to the function it fails silently (It looks complicated to log errors from the JS in R).
So I added a small step to 'sanitize' the overlays in input: If the input is a list of overlays we do not touch it, else we wrap the single overlay into a list.

In the following example, the two calls to display work correctly. Previously, only the last one would have worked.

bpmn_file <- system.file("examples/Email_Voting.bpmn", package = "bpmnVisualization")
overlay <- create_overlay("user_task_1", "Hello")
display(bpmn_file, overlay)
display(bpmn_file, list(overlay))

closes #32

@csouchet
Copy link
Contributor

csouchet commented Oct 12, 2021

I tested manually with:

  • display(bpmn_file, overlay)
    The diagram with the overlay is well displayed.

  • display(bpmn_file, list(overlay))
    The diagram with the overlay is well displayed.

@csouchet csouchet changed the title feat(overlay): Ensure overlays are passed as a list to the js function [FEAT] Ensure overlays are passed as a list to the js function Oct 12, 2021
csouchet
csouchet suggested changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@csouchet csouchet left a comment

Choose a reason for hiding this comment

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

Good job 👍🏻

However, the documentation update is missing:

#' display
...
#' @param overlays A list of elements to be added to the diagram's existing elements.
#'      Use overlay function to create an overlay object with content and relative position.

The overlays parameter must be a list or just one element, now 🙂

@tbouffard tbouffard added the enhancement New feature or request label Oct 12, 2021
csouchet
csouchet approved these changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@csouchet csouchet left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 🙂

@csouchet csouchet merged commit d995787 into process-analytics:main Oct 15, 2021
@csouchet csouchet added external contribution 👤 Pull requests provided by someone who is not a core maintainer hacktoberfest-accepted Accepted Pull Request during Hacktoberfest labels Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@csouchet csouchet csouchet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request external contribution 👤 Pull requests provided by someone who is not a core maintainer hacktoberfest-accepted Accepted Pull Request during Hacktoberfest

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEAT] Ensure overlays are passed as a list

3 participants

@alachambre @csouchet @tbouffard