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

Provide templates as replace menu entries #207

Merged
merged 4 commits into from
Dec 2, 2022
Merged

Provide templates as replace menu entries #207

merged 4 commits into from
Dec 2, 2022

Conversation

smbea
Copy link
Contributor

@smbea smbea commented Nov 30, 2022 •
edited
Loading

Closes #204

See comment #204 (comment)

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 30, 2022
@smbea smbea marked this pull request as draft November 30, 2022 08:37
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Nov 30, 2022
@smbea smbea mentioned this pull request Nov 30, 2022
Closed
nikku
nikku reviewed Nov 30, 2022
View reviewed changes
lib/camunda-cloud/ElementTemplatesReplaceProvider.js Outdated
...template,
group,
id: `replace-with-template-${id}`,
imageUrl: sanitizeImageUrl(icon.contents),
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we'd need to do this here? Feels like a core concern (to sanitize the image URL).

Copy link
Member

Choose a reason for hiding this comment

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

(We could of course decide element templates are stricter than the core).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to add it in the core. But I don't have a super strong opinion on this

@smbea smbea force-pushed the 204-add-replace-anything branch 2 times, most recently from 3719ce6 to b21e4f3 Compare December 1, 2022 11:57
@smbea smbea marked this pull request as ready for review December 1, 2022 12:02
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 1, 2022
@smbea smbea requested review from nikku, a team and barmac and removed request for a team December 1, 2022 19:59
@nikku nikku force-pushed the 204-add-replace-anything branch from b21e4f3 to 286a15b Compare December 2, 2022 09:45
nikku
nikku requested changes Dec 2, 2022
View reviewed changes
Copy link
Member

@nikku nikku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great overall. I verified via an integration test that it works as expected.

I'd ask you to do the following:

  • test via API, not DOM that integration works.
  • add an actual test to verify template -> popup menu entry conversion works as expected (including category)
  • consider adding a it.skip test case for the bug you found so we can pick this up as a follow up.

test/camunda-cloud/ElementTemplatesReplaceProviderSpec.js Outdated Show resolved Hide resolved
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 2, 2022
@smbea smbea force-pushed the 204-add-replace-anything branch from 286a15b to 6acd4a7 Compare December 2, 2022 10:28
smbea
smbea commented Dec 2, 2022
View reviewed changes
test/camunda-cloud/ElementTemplatesReplaceProviderSpec.js Outdated
Comment on lines 181 to 183
it('should have group - default', inject(function(popupMenu) {

// given
const templateEntryId = getTemplateEntries()[0];
const entry = popupMenu._getEntry(templateEntryId);

// then
expect(entry.group.id).to.eql('templates');
expect(entry.group.name).to.eql('Templates');

}));


it('should have group - category', inject(function(popupMenu) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already had this for group/category (reworked now to not use DOM). Were you envisioning something else ?

Copy link
Member

Choose a reason for hiding this comment

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

Missed that one, thanks :)

@smbea smbea requested a review from nikku December 2, 2022 10:31
nikku
nikku reviewed Dec 2, 2022
View reviewed changes
test/camunda-cloud/ElementTemplatesReplaceProviderSpec.js Outdated
isTemplateApplied
} from 'lib/camunda-cloud/features/replace/ElementTemplatesReplaceProvider';

import simpleXml from '../fixtures/replaceElementTemplates.bpmn';
Copy link
Member

Choose a reason for hiding this comment

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

Let's move test diagrams and implementation close to each other.

Name ElementTemplatesReplaceProvider.element-templates.json
Name ElementTemplatesReplaceProvider.bpmn.

This way we embrace locality (and ensure diagrams are not accidentally used by others).

Copy link
Member

Choose a reason for hiding this comment

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

I'll incorporate this.

Copy link
Member

Choose a reason for hiding this comment

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

I know we have fixtures in many places still, but it is an anti pattern (to move things away from each other).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know we were trying to move away from this so I was trying to respect it. But makes sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

Good example is bpmn-js, where we stared with a HUGE ball of fixtures, and now moved to a very "focused per test" fixture set. That worked much better.

nikku and others added 2 commits December 2, 2022 13:43
@nikku
deps!: update to bpmn-js@11 / diagram-js@11
dbbd271 
Incorporates new popup menu.

BREAKING CHANGES:

* Slight changes in popup menu look
@smbea @nikku
feat(cloud): templates as replace menu entries
831e485 
Closes #204
@nikku
test: be able to drop + open local diagrams
dcaf8ca 
Improves ability to integration test.
@nikku nikku force-pushed the 204-add-replace-anything branch from 6acd4a7 to 7db5e58 Compare December 2, 2022 13:12
@nikku nikku force-pushed the 204-add-replace-anything branch from 7db5e58 to 96abacd Compare December 2, 2022 13:12
@nikku nikku self-requested a review December 2, 2022 13:13
nikku
nikku approved these changes Dec 2, 2022
View reviewed changes
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Love how clean this is now (also testing the actual template entries, not DOM anymore).

Great work!

@smbea smbea merged commit 7adbaa4 into main Dec 2, 2022
@smbea smbea deleted the 204-add-replace-anything branch December 2, 2022 13:19
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Dec 2, 2022
@philippfromme philippfromme mentioned this pull request Jan 5, 2023
Closed
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku approved these changes

@barmac barmac Awaiting requested review from barmac barmac was automatically assigned from camunda/modeling-dev

Assignees

@smbea smbea

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add element templates to replace menu
2 participants
@smbea @nikku