-
Notifications
You must be signed in to change notification settings - Fork 33
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
[REFACTOR] Rename icon methods in IconPainter to reflect what is drawing #810
Conversation
@@ -354,7 +354,7 @@ export default class IconPainter { | |||
/** | |||
* This icon is used by `user task`. | |||
*/ | |||
public paintUserIcon({ c, ratioFromParent, setIconOrigin, shape, icon }: PaintParameter): void { | |||
public paintPersonIcon({ c, ratioFromParent, setIconOrigin, shape, icon }: PaintParameter): void { |
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.
❓ why changing this one? I don't get what is the clarification/generalization here. It has already been renamed recently, see https://github.com/process-analytics/bpmn-visualization-js/pull/522/files#diff-5e09027c60c84ce5c53f738dd6ad77bbc15f103d8e0b99de725c8424d7cc2339R307
Be aware that changing this will break the custom icon example
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.
For me, it represents more a person than a user.
In my opinion, the term 'User' is just to add context to a task.
This icon can be used anywhere (but not sure if we re-use it), but not in the context of 'User'.
Good catch for the examples 😉
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.
for general context Person sounds better - so it depends if we want to reuse it elsewhere or just for task-icon
I would even propose to name it paintFemalePersonIcon as it actually draws a female :)
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.
We changed paintWomanIcon
in paintUserIcon
in the past, in order to allow to a developper to change woman to man.
In my opinion the icon painter should reflect only what it paints. And the class/method who uses it should add the context.
Maybe, an intermerdiate method is missing in the task-shapes file to call the paintWomanIcon
(whatever its name). And the developper should override this missing method and not the woman method if he wants to use a man icon.
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.
that makes sense 👍
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.
So, do I rename it in paintWomanIcon
?
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.
I meant ok for eventual intermediate method and overriding
here it is OK to stay with Person, for me, you can merge as it is (just remember about examples mentioned by Thomas)
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.
c60fa88
to
d8a0b03
Compare
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.
LGTM
7964f69
to
b0c1716
Compare
b0c1716
to
b3b9207
Compare
No description provided.