Add Support for New Slider in Common Properties #1675

srikant-ch5 · 2024-01-24T14:34:04Z

Fixes : #1674

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Signed-off-by: srikant <[email protected]>

matthoward366

I think you'll need to add schema support in uihints for min/max. step should already be support but it can't be part of the value.

matthoward366 · 2024-01-24T16:47:12Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+    "title": "Slider with Min Max"
+  },
+  "current_parameters": {
+    "slider": {


This can only be "slider: 10`. We don't support an object like this.

matthoward366 · 2024-01-24T16:47:55Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+      "id": "slider_error",
+      "type": "object",
+      "required": true,
+      "default": {


We can't have a value like this. Should be default: 50 or something like that.

matthoward366 · 2024-01-24T16:48:57Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+            },
+            "control": "slider",
+            "class_name": "slider",
+            "slider": {


This doesn't follow any of our standards. I don't really even know how this works.

Signed-off-by: srikant <[email protected]>

srikant-ch5 · 2024-01-25T10:13:27Z

Hi @matthoward366 Please review the schema changes and slider component with changes related to above feedback.

Signed-off-by: srikant <[email protected]>

matthoward366

Once the code changes are done we'll need unit tests.

matthoward366 · 2024-01-25T17:38:40Z

canvas_modules/common-canvas/src/common-properties/controls/slider/slider.jsx

+		this.id = ControlUtils.getControlId(props.propertyId, this.uuid);
+		this.state = {
+			value: props.value,
+			min: props.control.min_value,


min, max, and step shouldn't be state values. These don't change. Does value also need to be in the state? We should be able to get this directly from the controller/redux.

matthoward366 · 2024-01-25T17:39:52Z

canvas_modules/common-canvas/src/common-properties/controls/slider/slider.jsx

+				data-id={ControlUtils.getDataId(this.props.propertyId)}
+			>
+				<Slider
+					id={this.id}


Is id required? From the docs it's not. In that case we can remove the code for it.

matthoward366 · 2024-01-25T17:40:29Z

canvas_modules/common-canvas/src/common-properties/controls/slider/slider.jsx

+				<Slider
+					id={this.id}
+					value={this.state.value}
+					min={this.state.min}


What happens in min, max, step aren't defined? Shouldn't we default them in the code?

matthoward366 · 2024-01-25T17:41:57Z

canvas_modules/common-canvas/src/common-properties/controls/slider/slider.jsx

+					labelText={this.props.controlItem}
+					onChange={this.handleChange}
+					disabled={this.props.state === STATES.DISABLED}
+					formatLabel={


Why does minLabel or maxLabel work?

Can you answer this?

matthoward366 · 2024-01-25T17:43:41Z

canvas_modules/common-canvas/src/common-properties/form/ParameterInfo.js

@@ -330,6 +336,8 @@ export class ParameterDef {
 				"orientation": propertyOf(uihint)("orientation"),
 				"width": propertyOf(uihint)("width"),
 				"charLimit": propertyOf(uihint)("char_limit"),
+				"min_value": propertyOf(uihint)("min_value"),


internally we usually use camalCase. So minValue and maxValue.

matthoward366 · 2024-01-25T17:44:16Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+    {
+      "id": "slider-disable",
+      "type": "boolean"
+	  },


Can you make sure to format the file?

matthoward366 · 2024-01-25T17:44:33Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+    },
+    {
+      "id": "slider_error",
+      "type": "object",


This shouldn't be an object

can we also test with double values. Something with 0 and 1 being the min/max with increments of 0.1

matthoward366 · 2024-01-25T17:45:45Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+          "min_value": 10,
+          "max_value": 100,
+          "increment": 10,
+          "test": "test",


This looks wrong. I don't think test is valid.

…ted to Slider component Signed-off-by: srikant <[email protected]>

Signed-off-by: srikant <[email protected]>

srikant-ch5 · 2024-01-30T11:22:42Z

Hi @matthoward366 Please review these changes and also I have included test cases.

matthoward366 · 2024-01-30T22:26:02Z

canvas_modules/common-canvas/src/common-properties/controls/slider/slider.scss

+        min-width: 8.6rem;
+    }
+    .bx--slider-text-input {
+        background-color: #F4F4F4;


Why do you need to set this? This should be controlled by light property on the control. It also needs to be set based on if it's in a tearsheet or right side flyout.

matthoward366 · 2024-01-30T22:26:15Z

canvas_modules/common-canvas/src/common-properties/controls/slider/slider.scss

+ */
+ .properties-slider {
+    .bx--slider {
+        min-width: 8.6rem;


Can you add a comment as to why this is needed?

matthoward366 · 2024-01-30T22:26:39Z

canvas_modules/common-canvas/src/common-properties/controls/slider/slider.jsx

+					labelText={this.props.controlItem}
+					onChange={this.handleChange}
+					disabled={this.props.state === STATES.DISABLED}
+					formatLabel={


Can you answer this?

matthoward366 · 2024-01-30T22:27:55Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+    "slider-disable": false
+  },
+  "parameters": [
+    {


Can you add more option? What happens if null or something undefined is passed in. We usually add these into the parameterDef to validate.

matthoward366 · 2024-01-30T22:28:24Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+        "min_value": 10,
+        "max_value": 100,
+        "increment": 10,
+        "test": "test",


Why is this here? This isn't valid for the schema.

matthoward366 · 2024-01-30T22:28:49Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+        },
+        "type": "controls",
+        "parameter_refs": [
+          "slider_error"


Can you add a test for warning?

matthoward366 · 2024-01-30T22:29:03Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+        "type": "controls",
+        "parameter_refs": [
+          "slider-disable",
+          "slider"


Can you add a hidden option?

Signed-off-by: srikant <[email protected]>

… Update Comment in scss Signed-off-by: srikant <[email protected]>

Signed-off-by: srikant <[email protected]>

…verage Signed-off-by: srikant <[email protected]>

matthoward366 · 2024-02-02T14:34:17Z

canvas_modules/common-canvas/src/common-properties/controls/slider/slider.scss

+		}
+	}
+	&.error {
+		input {


Should this also have the disabled check?

Signed-off-by: srikant <[email protected]>

matthoward366 · 2024-02-02T16:41:23Z

canvas_modules/harness/test_resources/parameterDefs/slider_paramDef.json

+          "default": "slider with parameter value set to 'null'"
+        },
+        "control": "slider",
+        "min_value": null,


These need to be removed. They aren't valid and cause schema validation errors.

Signed-off-by: srikant <[email protected]>

matthoward366

Looks good

elyra-ai#1674 : Add New Slider in Common Properties

ef951f4

Signed-off-by: srikant <[email protected]>

srikant-ch5 requested a review from matthoward366 January 24, 2024 14:34

srikant-ch5 mentioned this pull request Jan 24, 2024

Schema Changes for Slider Control elyra-ai/pipeline-schemas#138

Merged

matthoward366 requested changes Jan 24, 2024

View reviewed changes

srikant-ch5 marked this pull request as draft January 25, 2024 05:27

elyra-ai#1674 : Adjust Slider Schema to include min, max and step

237b427

Signed-off-by: srikant <[email protected]>

elyra-ai#1674 : Rename min and max to min_value and max_value

bfb5c72

Signed-off-by: srikant <[email protected]>

matthoward366 reviewed Jan 25, 2024

View reviewed changes

srikant-ch5 added 4 commits January 29, 2024 10:46

elyra-ai#1674 : rename to minValue and maxvalue and code changes rela…

fe9a27e

…ted to Slider component Signed-off-by: srikant <[email protected]>

elyra-ai#1674 : Include an newline in slider_paramDef

21513b8

Signed-off-by: srikant <[email protected]>

elyra-ai#1674 : Remove Slider ID

781a3ad

Signed-off-by: srikant <[email protected]>

Include unit testing for Slider component

714d797

Signed-off-by: srikant <[email protected]>

Merge branch 'main' into 15016_Slider

c9bf27c

matthoward366 reviewed Jan 30, 2024

View reviewed changes

srikant-ch5 added 11 commits January 31, 2024 19:26

Include different paramdef slider conditions

96b82f4

Signed-off-by: srikant <[email protected]>

Add slider size comment

0975e92

Signed-off-by: srikant <[email protected]>

Update slider size comment

4ec1f4d

Signed-off-by: srikant <[email protected]>

include slider small condition for smaller editor size

06f6067

Signed-off-by: srikant <[email protected]>

Merge branch 'main' into 15016_Slider

579401f

Improve Slider test coverage and fix slider scss identation issues

4c4a5db

Signed-off-by: srikant <[email protected]>

Make Slider Responsive by overriding default min-width of bx-slider

10dfb15

Signed-off-by: srikant <[email protected]>

Make Slider Responsive by overriding default min-width of bx-slider :…

77fb10d

… Update Comment in scss Signed-off-by: srikant <[email protected]>

Add separate paramdef for slider disable

541b2ca

Signed-off-by: srikant <[email protected]>

Improve slider test coverage to make it 100% and Truncate Long Labels

06f7f35

Signed-off-by: srikant <[email protected]>

Include Error and Warning state color border in slider and improve co…

21aef26

…verage Signed-off-by: srikant <[email protected]>

matthoward366 reviewed Feb 2, 2024

View reviewed changes

Restore Slider input borders and add disabled for error input

f9e3e21

Signed-off-by: srikant <[email protected]>

srikant-ch5 marked this pull request as ready for review February 2, 2024 15:08

srikant-ch5 added 5 commits February 2, 2024 20:45

Use minLabel and maxLabel and formatLabel when labels are not defined

1da7d94

Signed-off-by: srikant <[email protected]>

resolve label test case error

3925a68

Signed-off-by: srikant <[email protected]>

resolve slider value prop req error

e77c35e

Signed-off-by: srikant <[email protected]>

resolve slider value set it to min if null or undefined

bfc7089

Signed-off-by: srikant <[email protected]>

resolve slider value set it to min if null or undefined

6cfc65d

Signed-off-by: srikant <[email protected]>

matthoward366 reviewed Feb 2, 2024

View reviewed changes

srikant-ch5 added 3 commits February 2, 2024 22:15

resolve slider nulls from paramdefs

351f8c6

Signed-off-by: srikant <[email protected]>

remove validation props from slider

0c4d0bd

Signed-off-by: srikant <[email protected]>

remove focus not from slider error warning css

dcb1674

Signed-off-by: srikant <[email protected]>

matthoward366 approved these changes Feb 2, 2024

View reviewed changes

matthoward366 merged commit e71a2b4 into elyra-ai:main Feb 2, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for New Slider in Common Properties #1675

Add Support for New Slider in Common Properties #1675

srikant-ch5 commented Jan 24, 2024

matthoward366 left a comment

matthoward366 Jan 24, 2024

matthoward366 Jan 24, 2024

matthoward366 Jan 24, 2024

srikant-ch5 commented Jan 25, 2024 •

edited

Loading

matthoward366 left a comment

matthoward366 Jan 25, 2024

matthoward366 Jan 25, 2024

matthoward366 Jan 25, 2024

matthoward366 Jan 25, 2024

matthoward366 Jan 30, 2024

matthoward366 Jan 25, 2024

matthoward366 Jan 25, 2024

matthoward366 Jan 25, 2024

matthoward366 Jan 25, 2024

matthoward366 Jan 25, 2024

srikant-ch5 commented Jan 30, 2024

matthoward366 Jan 30, 2024

matthoward366 Jan 30, 2024

matthoward366 Jan 30, 2024

matthoward366 Jan 30, 2024

matthoward366 Jan 30, 2024

matthoward366 Jan 30, 2024

matthoward366 Jan 30, 2024

matthoward366 Feb 2, 2024

matthoward366 Feb 2, 2024

matthoward366 left a comment

Add Support for New Slider in Common Properties #1675

Add Support for New Slider in Common Properties #1675

Conversation

srikant-ch5 commented Jan 24, 2024

matthoward366 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikant-ch5 commented Jan 25, 2024 • edited Loading

matthoward366 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikant-ch5 commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthoward366 left a comment

Choose a reason for hiding this comment

srikant-ch5 commented Jan 25, 2024 •

edited

Loading