sartography / spiff-arena

SpiffWorkflow is a software development platform for building, running, and monitoring executable diagrams
https://www.spiffworkflow.org/
GNU Lesser General Public License v2.1
66 stars 43 forks source link

Compensation range field #647

Closed harmeet-status closed 3 months ago

harmeet-status commented 10 months ago

Implement a solution which would allow handling the compensation amount range: Position 2 input fields side by side, where one of them represents the minvalue and another one - maxvalue

To Do:

harmeet-status commented 10 months ago

Comment from @calexh-sar in Discord:

“discussed the slider with the devs this morning and appears the best approach is to modify what we have to include being able to set the min value as well by sliding. I suspect we would want to update both the integerRange and integerRangeSteps options since using the latter might be the better approach for the salary range use case so the user does't have to spend extra time trying to hit the exact amount. There was a question if a slider is the right choice for the salary range use case since the min and max values are likely to be known and the usefulness of a slider to tweak the range is probably not needed: https://www.smashingmagazine.com/2017/07/designing-perfect-slider/. But if you and/or Veronika feel this is the best approach, a quick estimate without digging deeper is it will be a Small t-shirt size ticket, 1-3 days, so we can maintain the Carbon look and feel.”

harmeet-status commented 10 months ago

Comment from @sashayar13:

I've been thinking also about just using two input fields, but these are the comments from KB and Alex “KB responded it's a cool idea, i think it's just harder than you might expect to implement and maybe on the order of 5 days. Not sure if it would fall into a Medium (3-5 dev days) or Large (5-10 dev days) t-shirt size, but suspect the latter.”

Image

harmeet-status commented 10 months ago

Comment from @sashayar13:

after the internal discussion with the team, we agreed that the option with 2 input fields is preferable. Can we pls schedule the dev work?

burnettk commented 10 months ago

@harmeet-status in terms of scheduling the dev work, please go ahead and prioritize this one relative to other status tickets.

harmeet-status commented 10 months ago

This one has been assigned to @Kayvon-Martinez

harmeet-status commented 10 months ago

@Kayvon-Martinez can you update the status of this when you have started working on this.

sashayar13 commented 9 months ago

@Kayvon-Martinez I noticed that if I make the numeric_range field required, it doesn't display the asterisk "*" symbol to the end user as it should

Image

sashayar13 commented 9 months ago

@Kayvon-Martinez I also noticed this validation error, which says nothing to the user (maybe we can change the text of the message?) ; it displayed to me when I tried to edit the request and re-submit the form ("Edit request" button). @calexh-sar, maybe you can help @Kayvon-Martinez with this error/ It was something similar to the errors we had previously

Image The process model https://dev.app.spiff.status.im/process-models/manage-talents:talent-acquisition-from-job-requisition-to-hiring:job-requisition:request-new-role

Kayvon-Martinez commented 9 months ago

Hi @sashayar13 . I will try to fix those issues. Thank you for letting me know.

jasquat commented 9 months ago

@sashayar13 I think the issue with allowing a range field to be marked as "required" is defining what that means. By default the values for both fields are 0 and neither can be set to null. This satisfies the normal requirement for a required field and isn't different between required and not required range fields.

sashayar13 commented 9 months ago

@jasquat I'm just wondering how we can indicate to the end users that they have to provide some values > 0? Currently, If I add the "compensation" field as required, it doesn't display the asterisk "*", however, it shows this validation error if I don't change the values:

image
jasquat commented 9 months ago

@sashayar13 I think that's a bug. 0 is allowed but the default value shown there doesn't actually set the value 0. It's actually null.

@Kayvon-Martinez maybe we should remove all default values here and display blank input on those text fields instead and then support showing required so it all makes sense.

Kayvon-Martinez commented 9 months ago

Hi @sashayar13 and @jasquat . I have made it so there is an asterisk when the field is required. I also implemented @jasquat 's suggestion so now the inputs are empty by default and the value is still undefined by default. These changes make it so that when the field is required you have to change the values or it will error and when it is not required you can not change it and receive no errors. Should I submit the PR?

burnettk commented 9 months ago

@Kayvon-Martinez sounds good, feel free to demo the updates tomorrow, too!

Kayvon-Martinez commented 9 months ago

Okay thanks @burnettk .

Kayvon-Martinez commented 9 months ago

Hi @sashayar13 . I have just pushed the changes to main.

sashayar13 commented 9 months ago

thanks guys, @Kayvon-Martinez @jasquat !

calexh-sar commented 9 months ago

@Kayvon-Martinez I am pushing this ticket to Ready for QA. Please move back to In Progress if I am jumping the gun.

dinithihj commented 8 months ago

Tested on dev.app (Version 319/321) and found below issues Used https://dev.app.spiff.status.im/process-models/manage-talents:talent-acquisition-from-job-requisition-to-hiring:job-requisition:request-new-role process model for testing.

  1. Cannot enter decimal values as min/max values

  2. If I type 0 it doesn't enter any value on the boxes and shows as blank, but if I type 00(two times) then it will display as 0 image

  3. I entered 1000 for both min and max values, Then clicked on the Next button. Then click on the "Edit Request" button and delete min value. Now min value is displayed as blank and I can click on the Next button and continue without seeing any error message. On the second page range is displayed as 0-1000 image image

  4. When I entered - it did not allow me to enter it. But when I enter -- (two times) It displayed one - sign and I can see minus values image

sashayar13 commented 8 months ago

thank you for checking @dinithihj @Kayvon-Martinez could you pls have a look at the comments above and fix the issues @dinithihj mentioned?

harmeet-status commented 8 months ago

@Kayvon-Martinez can you pls post regular updates on this ticket, the last one was from Sasha 3 weeks ago.

Kayvon-Martinez commented 7 months ago

I have fixed the issues with the numeric range field. The changes are here: https://github.com/sartography/spiff-arena/pull/925 Example Schema: Image


There were no changes to the uiSchema. I added support for "minimum" and "maximum" options and made some bug and usability fixes to the field. "minimum" and "maximum" are now required and they are what controls what values can be entered in the fields. If they are not specified then the process will error. If the values aren't what they should be according to the "minimum" and "maximum" values in the schema then the form will not validate unless the field is not required and the field hasn't been changed.

Kayvon-Martinez commented 6 months ago

I have made the error messages for the field more descriptive. The error message will now tell the user which value is wrong and why. For example: if the minimum value that can be entered is 10,000 (according to "minimum" in the jsonSchema) and the minimum value the user inputs is 1,000 there will be an error message that tells the user that value should be greater than or equal to 10,000. This change is in https://github.com/sartography/spiff-arena/pull/925.

dinithihj commented 6 months ago

@Kayvon-Martinez Can you please give me examples of JSON files (both schema and UI) for creating a numeric range field? I tried to create a new model using the samples we had, but got this error.

Process Model - https://dev.app.spiff.status.im/process-models/misc:qa:forms:test-numeric-range-field

image

Kayvon-Martinez commented 6 months ago

Hi @dinithihj . I am sorry for not including them after I added the minimum and maximum. Here are the example schemas.

Example jsonSchema: image

Example uiSchema: image

dinithihj commented 6 months ago

Tested on dev.app (version 351/353) using the model https://dev.app.spiff.status.im/process-models/misc:qa:forms:test-numeric-range-field Found below issues

  1. When displaying an error message, the Min and Max values have been overwritten by the error message. Screenshot 2024-03-15 123641

  2. The error icons are not responsive. Please see the screenshots below. In the first one, the Numeric Range Fields are displayed on 1320 x 858 pixels. In the second one, some other controls are displayed on 1320 x 858 pixels. The error icon on the other controls shows within the control, but on the Numeric Range Fields, the error icon is far away from the control. Also, in the Numeric Range Fields, the error icon for the "Minimum" value is much closer to the "Maximum" value. image image

  3. You can type letters for the minimum and maximum values. Even when you submit, it doesn't validate for letters. image

  4. This point is open for discussion. If the numeric field isn't required, you can just fill in either the minimum or maximum and then submit the form. No validation is performed during the form submission. When displaying data, the blank value is displayed as "None.". https://dev.app.spiff.status.im/i/546

Because this is a range field, I believe that if a user enters data, he should provide both the minimum and maximum values. If the field isn't required, they can simply ignore it without entering both the minimum and maximum, or if they want to enter data, they should provide both the minimum and maximum values. Open for suggestions image image

  1. I've set the min and max of one numeric range field as required. If I attempt to submit the form without filling in the required min/max , it shows a message saying, "Some fields are invalid. Please correct them before submitting the form." However, it doesn't exactly show what the error is. image image
sashayar13 commented 6 months ago

@Kayvon-Martinez, can you please accommodate the case where there is no "minimum" and/or "maximum" value for the range? Having those limits configured should not be a MUST requirement for the component—it should be Optional. Also, I still don't see a way how to configure that the "max" values should be a positive value.

For example, the compensation range field in the Request New Role process:

The current solution doesn't allow us to configure the numeric range field as needed for the compensation range.

FYI, @dinithihj , @madhurrya

harmeet-status commented 5 months ago

@calexh-sar Kayvon is off atm, is it possible to assign this to one of the devs for next sprint?

jasquat commented 5 months ago

We removed the requirement for min and max to be set. Here's an example of no max value but min has to be a positive number: https://github.com/sartography/sample-process-models/blob/sample-models-1/misc/numeric-range-example/numeric-range-schema.json. note that even when max is not set (like in this example), if you type 5 min and -3 max in the field, it will disallow it because 5 is greater than 0 but -3 is not greater than the min, 5. so it sort of seems like this might satisfy the "request new role" use case. we will deploy this code to dev.app in a little while so you'll be able to test to make sure.

sashayar13 commented 5 months ago

@jasquat thanks, it works! The only thing now is to align the positioning of the fields. If I set only min limit this is the UI I get:

image

Also, I couldn't find where the error message was configured. My worry is that it's hardcoded for the numeric_range widget. If this is the case, it should sound more generic since this widget can be used in other processes too

image
jasquat commented 5 months ago

@sashayar13 it looks like the error message is set by the field title and field id. So in this case its "Compensation (yearly), USD" and "compensation". I'll remove the id since that's redundant and we can leave the label. But the message should be generic.

We will look into fixing the formatting issue.

jasquat commented 5 months ago

@sashayar13 how does this look? In order to fix the layout issue I'm defaulting to the infinity symbol if a min or max if defined without the other. image

sashayar13 commented 5 months ago

thanks, @jasquat. It looks good!

sashayar13 commented 5 months ago

@dinithihj noticed that the range allows entering text there, even though it's set as a numeric type. We can type text in the compensation field and when I submit it with text it gives this error. I think it’s better to disable typing text there.

Image

@jasquat could you pls check this?

jasquat commented 5 months ago

@sashayar13 I don't think we can do that easily. Since commas are required it has to be a string and that allows for text to be entered as well. Anything that allows only numbers does not allow commas as far as I can tell.

Actually, we can't keep a user from entering text but we can invalidate it. PR #1451 does that. If a user enters text and then submits, an error will appear and will not submit the text.

sashayar13 commented 5 months ago

thanks @jasquat it works

dinithihj commented 5 months ago

Tested on dev.app (version 383/384) using the model https://dev.app.spiff.status.im/process-models/misc:qa:forms:test-numeric-range-field

Below are the issues found

  1. If I click submit without typing anything in the numeric range fields, nothing happens. I've tested this when the numeric range field is required and when it's not required. When it's required, it should show an error message, and when it's not required, the form should continue to the next task. image

  2. When displaying an error message, the Min and Max values have been overwritten by the error message. image

  3. Suggestion: When the minimum value is not set currently it is shown as minus infinity. However component allows only positive values. Therefore I think it'll be a bit confusing for the user. I would suggest showing the minimum value as 0 when it's not set.

  4. Suggestion: When checking on a monitor with a display resolution of 1920 x 1200, the error icon for the numeric range field appears far away from the input boxes. Is it possible to adjust it so that it's closer to the input boxes? Other controls seem to have their error icon inside the control. image image

jasquat commented 5 months ago

Addressed in #1464

1) Fixed 2) Fixed - I added the same min/max text as the invalid text in the component so it'll display as red text if the value is invalid. I think this is the most we can do since the carbon component removes the helperText if the field is invalid. 3) Fixed - bad regex 4) I'm not sure how much of this control and I would recommend not worrying about it since this component will change with the ui redesign.

dinithihj commented 5 months ago

Tested on dev.app (version 384/385) above issues are fixed now.

But now non-required fields are considered as required fields. if I click on submit without entering data into the non-required field it gives an error message "xxxxxxxxxxx must have valid numbers"

image

jasquat commented 5 months ago

@dinithihj that should be fixed in #1478

dinithihj commented 4 months ago

If I enter only maximum or minimum on a non-required numeric range field, it continues the process with the value "None" for not entered min/max. I'm wondering if this is right. I think because it's a field for a range, we should enter both the min/max numbers if we are entering values.

@sashayar13 do you think this behavior is ok or do we need to validate it?

image image

sashayar13 commented 4 months ago

@dinithihj Since this is a non-required field - we can't validate the value. The only suggestion I'd make is that when the "min" value is set (like you have an example where "min: 0"), if a user indicates nothing, this value should be populated by the system. So it would be Min: 0 instead of Min: None

burnettk commented 4 months ago

i can see why you'd suggest using a min value as a default, but i can also see the other side, like some users would probably be confused that for non-required fields sometimes the value is None and sometimes it is a value they didn't enter (however logical it may be, it's probably confusing).

dinithihj commented 4 months ago

Tested on test.app (version 212/214) using model - https://test.app.spiff.status.im/process-models/misc:qa:forms:test-numeric-range-field

All special characters have been validated properly and giving an error message as expected.

But If I enter "--" as the min/max value it is considered as None. (I was testing some negative values on the component and mistakenly entered two "-" and when displaying it was displayed as None. ) If I enter min value as 100 and max value as "--" for a required field it displays values 100 for min and None for max.

image image

dinithihj commented 4 months ago

Noticed another bug in the Numeric Range Field. If I try to submit the form without entering a value for a required numeric range field, it correctly asks me to fill in the values. However, if I enter a value for the required range field, then delete it, and submit the form, there is no validation. The form proceeds to the next step, and the required range field ends up with a value of 0.

Video Link :- https://drive.google.com/file/d/1pxDNxNczxMLFiVe6tVKrUEkMdnt2Ud9N/view?usp=sharing

dinithihj commented 4 months ago

For some reason, the validation for 0 isn't working right now. For example, if I set the minimum to 100 and the maximum to 1000, and enter 10 and 1000, it validates correctly. But if I enter 0 and 1000, it doesn't validate. Also, if I enter 0 for both the minimum and maximum, it doesn't validate correctly either.

Video Link:- https://drive.google.com/file/d/1KGZkPlUCfUzHaEaW18BvS9nJ8I2AlgO-/view?usp=sharing

dinithihj commented 3 months ago

Tested and verified on test.app(version 214/216) using the model https://test.app.spiff.status.im/process-models/misc:qa:forms:test-numeric-range-field

The above issues seem to be fixed now.

dinithihj commented 3 months ago

Released to Prod on June 18th. Version 19/20.