-
Notifications
You must be signed in to change notification settings - Fork 365
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.
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
… I removed suffix
changed setting state in EditUnitModalComponent to be consistent with CreateUnitModalComponent
- Keep state declaration at top of unit modals - Place isCustomRate in utils to share - reorder some lines of code so consistent - Remove comment on edit reset in other files - Other minor edits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gyordong The changes you made looked good and worked in almost every case. Since it was complex to note all my ideas and to move this forward, I pushed a commit that aligned unit edit and create. With this, I think this PR is now ready to merge. I'll let it sit for a little while to give everyone involved a chance to look at what I did and comment if they have nay concerns. If nothing needs changing then it will be merged as is. Congratulations on completing this work and I appreciate your efforts to do address all the comments given changes over time.
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