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

2042 - enable connection tool for text annotation #1428

Merged
merged 17 commits into from
Apr 7, 2021
Merged

2042 - enable connection tool for text annotation #1428

merged 17 commits into from
Apr 7, 2021

Conversation

tkhadir
Copy link
Contributor

@tkhadir tkhadir commented Mar 31, 2021

Build Status

🔗 linked to camunda modeler issue : camunda/camunda-modeler#2042
purpose : enable connection tool to text annotation
modified :

  • BpmnRules.js to be able start link from annotation to other component
  • ContextPafProvider.js to add connectTool in the menu in addition to the remove icon

image

@MaxTru
Copy link
Contributor

MaxTru commented Apr 5, 2021

Hi @tkhadir ,

thanks a lot for your contribution! 👏

Before continuing: could you please:

  1. Add tests for the added feature (take a look at https://github.com/bpmn-io/bpmn-js/blob/develop/test/spec/features/rules/BpmnRulesSpec.js#L2258 and https://github.com/bpmn-io/bpmn-js/blob/develop/test/spec/features/context-pad/ContextPadProviderSpec.js)
  2. Ensure that the commit messages adhere to our to the conventional commits guidelines

Thanks!

–––––––––––––
Context: https://github.com/bpmn-io/bpmn-js/blob/develop/.github/CONTRIBUTING.md

@pinussilvestrus pinussilvestrus added the help wanted Extra attention is needed label Apr 6, 2021
@tkhadir
Copy link
Contributor Author

tkhadir commented Apr 6, 2021

hello @MaxTru,

I have update tests files according to the recommandations, thank you very much for your feedback 😄

Copy link
Contributor

@MaxTru MaxTru left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests. I added some comments after reviewing.

test/spec/features/context-pad/ContextPadProviderSpec.js Outdated Show resolved Hide resolved
test/spec/features/context-pad/ContextPadProviderSpec.js Outdated Show resolved Hide resolved
lib/features/context-pad/ContextPadProvider.js Outdated Show resolved Hide resolved
@MaxTru MaxTru added fixed upstream Requires integration of upstream change needs review Review pending and removed fixed upstream Requires integration of upstream change labels Apr 7, 2021 — with bpmn-io-tasks
tkhadir and others added 4 commits April 7, 2021 10:18
Co-authored-by: MaxTru <42800119+MaxTru@users.noreply.github.com>
Co-authored-by: MaxTru <42800119+MaxTru@users.noreply.github.com>
@MaxTru
Copy link
Contributor

MaxTru commented Apr 7, 2021

There is a linting error in ContextPadProvider

/home/maxtru/Dokumente/10_modules/bpmn-io/bpmn-js/lib/features/context-pad/ContextPadProvider.js
  446:1  error  Parsing error: Unexpected token

  444 | 
  445 |   return actions;
> 446 | };
      | ^
  447 | 
  448 | 
  449 | // helpers /////////

@tkhadir
Copy link
Contributor Author

tkhadir commented Apr 7, 2021

hello @MaxTru it seems good now 😄 thanks again for your feedback

@MaxTru MaxTru merged commit c41f7d5 into bpmn-io:develop Apr 7, 2021
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Apr 7, 2021
@MaxTru
Copy link
Contributor

MaxTru commented Apr 7, 2021

Thanks a lot for the contribution, good job!

I squashed the commits together so to have a clean commit history: for the next time, please also try to only provide a handful (or even just one) reasonable commit, this makes it easier to review the change.

@tkhadir
Copy link
Contributor Author

tkhadir commented Apr 7, 2021

thanks lot @MaxTru for your great help 😄

pinussilvestrus pushed a commit to camunda/camunda-bpmn-js that referenced this pull request Apr 8, 2021
merge-me bot pushed a commit to camunda/camunda-bpmn-js that referenced this pull request Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants