Custom rate and named standard rate for units (Issue #1216) #1388
Conversation
…idation for integers in custom value
… to hour on dropdown
…anslation for unit.sec.in.rate.enter
…t value to custom input if nonstandard value
- Update unit edit logic for useEffect more - Change custom sec in rate to require enter - Disable save if not changed and incomplete custom input - use setState and not directy set state - consistently use === & !== - Added SHL comments for original developers to look at and then remove - Formatting - Minor edit Note create unit does not have new logic and bar may need to be update to be similar
Note that @gyordong worked with me on these changes. - Also has comments with ?? on items to do. - Not carefully tested/completed.
…ts on customratevalid(), included customratevalid in editunit's canSave useEffect check to fix a bug with negative numbers.
gyordong
changed the title
There was a problem hiding this comment.
I want to thank @gyordong, @aduques, @AchsahJojo & Emily Sarne for this contribution. Is Emily Sarne the GitHub user @emilysarne? I want to verify they have signed the CLA if they contributed to this work.
Testing found this works as desired. Most of my comments relate to differences between the code for edit and create for unit. I welcome your thoughts on my comments and you can address if you think a comment is fine. Please let me know if I can help in any way.
| if (state.typeOfUnit != UnitType.suffix && state.suffix != '') { | ||
| state.typeOfUnit = UnitType.suffix; | ||
| } | ||
| const submitState = { |
There was a problem hiding this comment.
While the change in how state is set here is nice, edit unit differs. This change should probably go there too. Note identifier is set here and done after save in edit so make consistent. I think I did this so it probably isn't your doing. If you prefer, I can make the change.
There was a problem hiding this comment.
changed setting state in EditUnitModalComponent to be consistent with CreateUnitModalComponent
| // - The rate is set so not the custom input value. This happens if select custom value but don't input with enter. | ||
| // - The custom rate is a positive integer | ||
| const validUnit = state.name !== '' && | ||
| (state.typeOfUnit !== UnitType.suffix || state.suffix !== '') && state.secInRate !== Number(CUSTOM_INPUT) |
There was a problem hiding this comment.
This checks for not hitting enter on custom but create does not. This is kind to the user and nice to have.
There was a problem hiding this comment.
After looking at comments on the CreateUnitModalComponent, I noticed that the design of the current implementation was to discard all values when leaving the page without saving. Given this, the custom rate would be discarded alongside the other values despite changing the similar line of code in CreateModalComponent. Should CreateUnitModalComponent be changed to still keep unsaved changes similarly to how it is implemented in EditUnitModalComponent?
There was a problem hiding this comment.
I thought this test was to see if the user had started a custom input but not hit enter. This likely indicates they started to enter a value but never finalized it with an enter. This seems a potentially common mistake that OED is trying to catch to avoid. I didn't think it has to do with leaving the page and resetting state but maybe I'm missing something. What do you think?
There was a problem hiding this comment.
Hi Steve, I changed the code to check for enter. The only caveat is that I made it purposely allow users to save without clicking enter if they didn't change the custom value from the standard values automatically inputted when clicked (ex. day to custom value, box shows 86400, create unit can be clicked). Let me know if this doesn't seem ideal and I'll change it to require enter to create a unit under all circumstances.
There was a problem hiding this comment.
Sorry I missed this comment until now. I'm not sure which is best. It is unclear why the admin selected custom if they did not intend to change the value. However, I'm okay with this as long as both create and edit work the same.
There was a problem hiding this comment.
I think the difference won't be noticeable for the user. Should I change it to require a change of value or is it fine as is?
There was a problem hiding this comment.
I'm not sure if I understand but it is okay to use the value without enter if it is the standard value as long as both create and edit do the same thing. Is this what you were asking?
There was a problem hiding this comment.
Yes. Do you have a preference between the two implementations?
There was a problem hiding this comment.
I don't care too much but I might pick that if they do custom then they have to hit enter. One case I am thinking about is if they change the value away from a standard one and then put it back without hitting enter then would the code accept this second value because it is standard? This type of issue is why I'm leaning toward requiring enter for all custom values but maybe it isn't an issue.
… I removed suffix
changed setting state in EditUnitModalComponent to be consistent with CreateUnitModalComponent
Description
(Please include a summary of the change and which issue is touched on. Please also include relevant motivation and context.)
Fixes #1216
Done in collaboration with @huss, @aduques, @AchsahJojo, @emilysarne.
Type of change
Checklist
Limitations