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

feat(style)!: refactor how to build the overlay style #245

Merged
merged 10 commits into from
Sep 11, 2023

Conversation

csouchet
Copy link
Member

@csouchet csouchet commented Sep 8, 2023

To address issue #13 efficiently and avoid creating separate functions for creating font, fill and stroke styles for overlay, edge and shape, I refactored the existing code.

As part of this refactoring, I've converted the create_font, create_fill and create_stroke functions to internal functions.
This enhancement allows these functions to accept additional parameters as needed. The responsibility for passing the appropriate parameters to these functions now rests with the functions dedicated to overlays (create_overlay_style), edges, and shapes.
 

BREAKING CHANGE: create_font, create_fill and create_stroke functions are internal functions. create_style has been renamed in create_overlay_style, and now, we use only this function to create the style of an overlay.

@csouchet csouchet added documentation Improvements or additions to documentation refactoring Code refactoring labels Sep 8, 2023
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

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

🤖 By surge-preview

@csouchet csouchet marked this pull request as ready for review September 8, 2023 09:46
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.

⚠️ This PR introduces breaking changes, so its title and content should mention it. I suggest something like refactor!: simplify the construction of the overlay style for the PR title.
The commit used to merge the PR must also mention it and the release notes as well.
See https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with-both--and-breaking-change-footer

I don't know how breaking changes have to be handled in the R ecosystem. This is something that should be checked IMHO.

I don't see any problem for introducing breaking changes in general. Can you please give me an example of the future "style update" functions to let me understand how the refactoring help for the new API?
In parallel, I am reviewing the details of the code 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.

Very good job 💪🏿, and thank you for your efforts to improve our documentation.
I made some minor suggestions but the most important things to me is about handling the breaking changes introduced by this PR.

@@ -38,35 +38,38 @@
#' # Load the BPMN file
#' bpmn_file <- system.file("examples/Order_Management.bpmn", package = "bpmnVisualizationR")
#'
#' # Display the BPMN diagram
#' # Example 1: Display the BPMN diagram
Copy link
Member

Choose a reason for hiding this comment

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

praise: Nice, this makes things easier to understand when reading the documentation.

R/funs.R Outdated Show resolved Hide resolved
R/funs.R Outdated Show resolved Hide resolved
R/funs.R Outdated Show resolved Hide resolved
#' Refer to the \code{font} parameter in the \code{\link{create_style}} function for more information.
#'
#' Use this function to create the correct font structure for an overlay.
#' - Overlay:
Copy link
Member

Choose a reason for hiding this comment

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

question: I guess this is added to prepare the new "update style" API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed 😊

R/funs.R Outdated Show resolved Hide resolved
R/funs.R Outdated Show resolved Hide resolved
R/funs.R Show resolved Hide resolved
@csouchet csouchet added the enhancement New feature or request label Sep 11, 2023
@csouchet csouchet changed the title refactor(style): refactor how to build the overlay style feat(style): refactor how to build the overlay style Sep 11, 2023
@csouchet csouchet changed the title feat(style): refactor how to build the overlay style feat(style)!: refactor how to build the overlay style Sep 11, 2023
@csouchet csouchet force-pushed the refactor_how_to_build_the_overlay_style branch from c51164c to 41db251 Compare September 11, 2023 09:03
@csouchet csouchet merged commit 4d9b000 into main Sep 11, 2023
4 checks passed
@csouchet csouchet deleted the refactor_how_to_build_the_overlay_style branch September 11, 2023 12:20
@tbouffard tbouffard added enhancement New feature or request and removed documentation Improvements or additions to documentation enhancement New feature or request refactoring Code refactoring labels Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants