fix(form): hide select options for disabled engines #2534

smbea · 2021-10-26T11:02:41Z

Closes #2512

Removes disabled engine options and checkbox when there is only one choice.

Screenshot:

andreasgeier

✅ LGTM

MaxTru

In general: looks great.

I have two issues / bugs:

Start with --disable-zeebe
Note that you can still create a Form with Cloud profile (:warning: this would be bug no.1, probably to be fixed seperately)
Now, as soon as I created a "Cloud" Form (probably also if I would open an existing Cloud form), I cannot switch the profile to platform (:warning: so this is bug no.2, directly related to this PR I think):

WDYT?

MaxTru · 2021-10-28T07:10:50Z

client/src/app/tabs/EngineProfile.js

              return <EngineProfileOption
                engineProfile={ engineProfile }
                key={ engineProfile.executionPlatform }
                onSelectEngineProfile={ onSelectEngineProfile }
-                selectedEngineProfile={ selectedEngineProfile } />;
+                selectedEngineProfile={ selectedEngineProfile }
+                onlyEngine={ enabledEngineOptions.length>1 ? false : true } />;


Suggested change

onlyEngine={ enabledEngineOptions.length>1 ? false : true } />;

onlyEngine={ enabledEngineOptions.length > 1 ? false : true } />;

smbea · 2021-10-28T11:13:59Z

Now, as soon as I created a "Cloud" Form (probably also if I would open an existing Cloud form), I cannot switch the profile to platform (⚠️ so this is bug no.2, directly related to this PR I think):

@MaxTru In this case, should it allow to change to Platform and then not allow to go back to Cloud? or maintain both?

MaxTru · 2021-10-28T11:29:38Z

Now, as soon as I created a "Cloud" Form (probably also if I would open an existing Cloud form), I cannot switch the profile to platform (warning so this is bug no.2, directly related to this PR I think):

@MaxTru In this case, should it allow to change to Platform and then not allow to go back to Cloud? or maintain both?

Well in this case we have --disable-zeebe but we opened a zeebe diagram right? So I think:

Opening a zeebe form is still valid and should work (when double-clicking a file). It should still show the right profile. (This is as-is / already works) ✔️
However, when I opened a zeebe form and then click on platform, it should allow to switch to platform. (This does not work, see gif ⚠️ in my original post)
Also, it should not be possible to create a new zeebe form from the menu ⚠️

Does that make sense?

smbea · 2021-10-28T11:57:47Z

However, when I opened a zeebe form and then click on platform, it should allow to switch to platform. (This does not work, see gif ⚠️ in my original post)

My want to confirm what happens after I change to platform.

I open a zeebe form and change engine to Platform
After that, what options appear? Just Platform and the Cloud option disappears? -> example bellow:

Screen.Recording.2021-10-28.at.13.54.46.mov

MaxTru · 2021-10-28T12:53:08Z

However, when I opened a zeebe form and then click on platform, it should allow to switch to platform. (This does not work, see gif warning in my original post)

My want to confirm what happens after I change to platform.

I open a zeebe form and change engine to Platform

After that, what options appear? Just Platform and the Cloud option disappears? -> example bellow:

Screen.Recording.2021-10-28.at.13.54.46.mov

Yes - I would confirm that. The recording looks good to me.

smbea · 2021-10-28T13:47:32Z

I addressed point 3 according to the recording I sent.
I also fixed point 2, even though it wasn't in the scope of this PR, because it was a simple fix.

MaxTru

Looks good! I only have three minor things, afterwards => good to merge!

MaxTru · 2021-10-29T06:56:22Z

client/src/app/tabs/EngineProfile.js

@@ -139,6 +143,15 @@ function EngineProfileSelection(props) {
      return;
    }

+    if (filterEngineOptions().length===1 && isEngineDisabled(selectedEngineProfile)) {


We already called the filterEngineOptions() before here

Suggested change

if (filterEngineOptions().length===1 && isEngineDisabled(selectedEngineProfile)) {

if (enabledEngineOptions.length === 1 && isEngineDisabled(selectedEngineProfile)) {

MaxTru · 2021-10-29T06:58:42Z

client/src/app/tabs/EngineProfile.js

              return <EngineProfileOption
                engineProfile={ engineProfile }
                key={ engineProfile.executionPlatform }
                onSelectEngineProfile={ onSelectEngineProfile }
-                selectedEngineProfile={ selectedEngineProfile } />;
+                selectedEngineProfile={ selectedEngineProfile }
+                onlyEngine={ enabledEngineOptions.length > 1 ? false : true } />;


In line 146 we check explicitly for length === 1, so should we not apply the same logic here as well? Is there a reason to be inconsistent?

Suggested change

onlyEngine={ enabledEngineOptions.length > 1 ? false : true } />;

onlyEngine={ enabledEngineOptions.length === 1 } />;

We should. I changed this one to match line 146

MaxTru · 2021-10-29T06:59:56Z

client/src/app/tabs/__tests__/EngineProfileSpec.js

@@ -237,6 +239,70 @@ describe('<EngineProfile>', function() {
    });


+    describe('show not show disabled engine profile', function() {


Suggested change

describe('show not show disabled engine profile', function() {

describe('not show disabled engine profile', function() {

Closes #2512

MaxTru

🎉

closes #2534

bpmn-io-tasks bot added the needs review Review pending label Oct 26, 2021

smbea requested review from a team, andreasgeier, philippfromme and barmac and removed request for a team October 26, 2021 11:03

andreasgeier approved these changes Oct 27, 2021

View reviewed changes

MaxTru requested changes Oct 28, 2021

View reviewed changes

barmac mentioned this pull request Oct 28, 2021

Add space-related rules bpmn-io/eslint-plugin-bpmn-io#12

Merged

smbea force-pushed the 2512-remove-disabled-engine-for-forms branch from 574da44 to 8e25113 Compare October 28, 2021 13:45

smbea requested a review from MaxTru October 28, 2021 13:48

MaxTru requested changes Oct 29, 2021

View reviewed changes

smbea added 2 commits October 29, 2021 09:12

fix(form): hide select options for disabled engines

0adc172

Closes #2512

fix(form): remove cloud form from menu when disabled

1587dbb

smbea force-pushed the 2512-remove-disabled-engine-for-forms branch from 8e25113 to 1587dbb Compare October 29, 2021 07:13

smbea requested a review from MaxTru October 29, 2021 07:16

MaxTru approved these changes Oct 29, 2021

View reviewed changes

MaxTru merged commit d8da764 into develop Oct 29, 2021

MaxTru deleted the 2512-remove-disabled-engine-for-forms branch October 29, 2021 07:52

bpmn-io-tasks bot removed the needs review Review pending label Oct 29, 2021

MaxTru added a commit that referenced this pull request Nov 30, 2021

deps(client): bump to camunda-bpmn-js@0.12.2

7876eb8

closes #2534

bpmn-io-tasks bot added the in progress Currently worked on label Nov 30, 2021

bpmn-io-tasks bot assigned MaxTru Nov 30, 2021

MaxTru mentioned this pull request Nov 30, 2021

deps(client): bump to camunda-bpmn-js@0.12.2 #2601

Merged

bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Nov 30, 2021

smbea mentioned this pull request Dec 2, 2021

Cherry pick changes for 4.12 release #2605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(form): hide select options for disabled engines #2534

fix(form): hide select options for disabled engines #2534

smbea commented Oct 26, 2021

andreasgeier left a comment

MaxTru left a comment •

edited

Loading

MaxTru Oct 28, 2021

smbea commented Oct 28, 2021

MaxTru commented Oct 28, 2021 •

edited

Loading

smbea commented Oct 28, 2021

MaxTru commented Oct 28, 2021

smbea commented Oct 28, 2021

MaxTru left a comment

MaxTru Oct 29, 2021

MaxTru Oct 29, 2021

smbea Oct 29, 2021

MaxTru Oct 29, 2021

MaxTru left a comment

	onlyEngine={ enabledEngineOptions.length>1 ? false : true } />;
	onlyEngine={ enabledEngineOptions.length > 1 ? false : true } />;

	if (filterEngineOptions().length===1 && isEngineDisabled(selectedEngineProfile)) {
	if (enabledEngineOptions.length === 1 && isEngineDisabled(selectedEngineProfile)) {

		@@ -237,6 +239,70 @@ describe('<EngineProfile>', function() {
		});


		describe('show not show disabled engine profile', function() {

	describe('show not show disabled engine profile', function() {
	describe('not show disabled engine profile', function() {

fix(form): hide select options for disabled engines #2534

fix(form): hide select options for disabled engines #2534

Conversation

smbea commented Oct 26, 2021

andreasgeier left a comment

Choose a reason for hiding this comment

MaxTru left a comment • edited Loading

Choose a reason for hiding this comment

MaxTru Oct 28, 2021

Choose a reason for hiding this comment

smbea commented Oct 28, 2021

MaxTru commented Oct 28, 2021 • edited Loading

smbea commented Oct 28, 2021

MaxTru commented Oct 28, 2021

smbea commented Oct 28, 2021

MaxTru left a comment

Choose a reason for hiding this comment

MaxTru Oct 29, 2021

Choose a reason for hiding this comment

MaxTru Oct 29, 2021

Choose a reason for hiding this comment

smbea Oct 29, 2021

Choose a reason for hiding this comment

MaxTru Oct 29, 2021

Choose a reason for hiding this comment

MaxTru left a comment

Choose a reason for hiding this comment

MaxTru left a comment •

edited

Loading

MaxTru commented Oct 28, 2021 •

edited

Loading