-
Notifications
You must be signed in to change notification settings - Fork 15
Add allowAdd prop to CropSelector to disable "+" button on Harvest form #308
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
base: development
Are you sure you want to change the base?
Conversation
* Commit for draft pull request * Removed + from the crop selector
* add allowAdd prop * add allowAdd prop * allowAdd prob done * no new crop can be added --------- Signed-off-by: rohanpiya <piyar@dickinson.edu> Co-authored-by: rohanpiya <piyar@dickinson.edu>
braughtg
left a comment
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.
Overall this is looking pretty good. There are some changes that have been requested.
In addition, the E2E tests for the Harvest form should confirm that the "+" button is not visible when the CropSelector is used on the Harvest form. The component test requested checks that the new prop works, this test will check that that prop has actually been set by the Harvest form.
Please make the requested changes and push them to this PR. When you have made the changes change the PR back to "Ready for review".
braughtg
left a comment
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.
This is getting close! I have one suggestion in the code for a change in phrasing. Also, the prior review asked for an E2E test. See the comment on the harvest.e2e.cy.js file.
When you have made the changes, respond briefly to each comment and then change this PR back to ready for review.
| default: false, | ||
| }, | ||
| /** | ||
| * Whether the "+" should appear on the crop dropdown menu. |
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 think a better phrasing here would be "Whether the user is allowed to add a new crop or not."
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.
As indicated in the prior review, a test should be added to this file that checks that the CropSelector is displayed in the Harvest form without the "+" allowing the user to add a new crop.
Purpose
This pull request updates the CropSelector component so that the green “+” button for adding a new crop can be optionally disabled.
In particular, the Harvest form should not allow adding a new crop from the CropSelector, since a crop must already exist in order to be harvested. This PR adds a prop to CropSelector to control that behavior and sets it so that the “+” is hidden on the Harvest form while remaining available in other contexts.
Verification steps
Navigate to: FarmData2 → Harvest. Locate the Crop input on the Harvest form.
Confirm that:
Approach
Testing
test.bash --fd2 --live --e2e --glob=modules/farm_fd2/**/harvest/*.e2e.cy.js --guiAll existing Harvest E2E tests passed.
Related issues
Close #279
Additional Information
H12 - OSS2 Submission. Gracie and Rohan
Licensing Certification
FarmData2 is a Free Cultural Work and all accepted contributions are licensed as described in the LICENSE.md file. This requires that the contributor holds the rights to do so. By submitting this pull request I certify that I satisfy the terms of the Developer Certificate of Origin for its contents.