-
Notifications
You must be signed in to change notification settings - Fork 4
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
[OSDEV-1653] Highlight the mandatory fields on the form for Search by name and address. #509
[OSDEV-1653] Highlight the mandatory fields on the form for Search by name and address. #509
Conversation
📝 WalkthroughWalkthroughThe changes refactor the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx (1)
40-45
: Add PropTypes validation for FormFieldTitle component.The component looks good, but it's missing PropTypes validation for its props.
Add the following PropTypes:
+FormFieldTitle.propTypes = { + label: PropTypes.string.isRequired, + classes: PropTypes.shape({ + formFieldTitleStyles: PropTypes.string.isRequired, + requiredAsterisk: PropTypes.string.isRequired + }).isRequired +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx
(4 hunks)src/react/src/util/styles.js
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
🔇 Additional comments (2)
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx (1)
127-127
: Well-implemented form field titles with required field indicators.The consistent use of FormFieldTitle component across all form fields improves user experience by clearly indicating which fields are mandatory.
Also applies to: 155-155, 181-181
src/react/src/util/styles.js (1)
990-997
: Good styling implementation for form field titles and required indicators.The style changes are well-structured:
- Semantic renaming from
subTitleStyles
toformFieldTitleStyles
- Consistent use of color constants for the required field indicator
React App | Jest test suite - Code coverage reportTotal: 29.55%Your code coverage diff: 0.54% ▴ ✅ All code changes are covered |
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.91%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/react/src/__tests__/components/SearchByNameAndAddressTab.test.js (1)
128-162
: Consider adding more assertions for form state changes.While the test covers the basic flow, consider adding assertions to verify intermediate form states:
- After each field is filled
- Form validation state changes
fireEvent.change(nameInput, { target: { value: "Test Name" } }); +expect(nameInput).toHaveValue("Test Name"); fireEvent.change(addressInput, { target: { value: "Test Address" } }); +expect(addressInput).toHaveValue("Test Address"); fireEvent.change(countrySelect, { target: { value: "US" } }); +expect(countrySelect).toHaveValue("US");src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx (1)
20-52
: Consider extracting style logic to a separate file.The
getSelectStyles
function contains complex styling logic. Consider moving it to a separate styles file for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
doc/release/RELEASE-NOTES.md
(1 hunks)src/react/src/__tests__/components/SearchByNameAndAddressTab.test.js
(1 hunks)src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx
(8 hunks)src/react/src/util/styles.js
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/util/styles.js
🧰 Additional context used
🧠 Learnings (1)
src/react/src/__tests__/components/SearchByNameAndAddressTab.test.js (2)
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#483
File: src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js:0-0
Timestamp: 2025-01-17T16:12:18.285Z
Learning: In the SearchByNameAndAddressSuccessResult component's tests, attempting to test internal navigation logic through mocking useHistory is not feasible, and button presence/click events should be covered in the main rendering test case.
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#483
File: src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js:110-118
Timestamp: 2025-01-17T15:04:49.076Z
Learning: In the SearchByNameAndAddressSuccessResult component, the Select functionality is implemented internally within the component rather than being passed as props, making it unsuitable for direct function mocking in tests.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-django-code-quality
🔇 Additional comments (7)
src/react/src/__tests__/components/SearchByNameAndAddressTab.test.js (4)
20-42
: LGTM! Well-structured mock implementation for StyledSelect.The mock implementation correctly handles all necessary props and events, making it suitable for testing the component's interaction with the select input.
60-77
: LGTM! Loading state test is properly implemented.The test verifies the presence of a loading indicator when the fetching state is true.
79-96
: LGTM! Error state test is properly implemented.The test correctly verifies error message rendering when an error occurs.
164-195
: LGTM! Comprehensive validation test.The test thoroughly checks error indication behavior for empty required fields.
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx (2)
54-59
: LGTM! Well-structured FormFieldTitle component.The component effectively encapsulates the rendering of field titles with required indicators.
89-97
: LGTM! Clear and focused event handlers.The blur event handlers are well-organized and maintain single responsibility.
doc/release/RELEASE-NOTES.md (1)
70-71
: LGTM! Clear documentation of UI changes.The release notes clearly document the changes related to required fields and error handling in the search form.
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.
Left few comments.
src/react/src/__tests__/components/SearchByNameAndAddressTab.test.js
Outdated
Show resolved
Hide resolved
src/react/src/__tests__/components/SearchByNameAndAddressTab.test.js
Outdated
Show resolved
Hide resolved
src/react/src/__tests__/components/SearchByNameAndAddressTab.test.js
Outdated
Show resolved
Hide resolved
src/react/src/__tests__/components/SearchByNameAndAddressTab.test.js
Outdated
Show resolved
Hide resolved
|
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.
Approved.
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.
LGTM
OSDEV-1653 - SLC. Implement search page for name & address search (FE). - Mandatory fields are not obvious.
In this PR, the following changes were implemented:
SearchByNameAndAddressTab
component was covered by test cases.